WaitForSingleObject signalled w/o SetEvent?
Michael Bruschkewitz
I've a strange problem w/ WFSO in the appendend small demo:
There are 2 treads, thread 1 increases randomly q.
If q was 0, Event ev is set.
Thread 2 counts down q, if q is 0, it waits for ev.
But, for some reasons, WFSO returns WAIT_OBJECT_O even
if q is still 0, what IMHO should never happen in the following code.
Using this "goto again" or a similar construct all works fine,
but why do I need this?
Can somebody explain this behaviour?
Regards,
Michael B.
#include <windows.h>
int q=0; CRITICAL_SECTION cs; HANDLE ev;
DWORD WINAPI stWriteThread(void*) {
for(int i=0; i<100; ++i) {
Sleep(rand()%256); int n = rand()%8;
for(int j=0; j<n; ++j) {
EnterCriticalSection(&cs);
q++; if (q==1) { SetEvent(ev); } // <-- Set
LeaveCriticalSection(&cs);
}
}
EnterCriticalSection(&cs);
SetEvent(ev);
LeaveCriticalSection(&cs);
MessageBox(0,"write","finished",MB_ICONINFORMATION);
return 0;
}
DWORD WINAPI stReadThread(void*) {
EnterCriticalSection(&cs);
for(;;) {
LeaveCriticalSection(&cs); EnterCriticalSection(&cs);
if (q==0) {
again: LeaveCriticalSection(&cs);
DWORD rc = WaitForSingleObject(ev,INFINITE);
EnterCriticalSection(&cs);
if (rc != WAIT_OBJECT_0) {
MessageBox(0,"read","wfso failed",
MB_ICONSTOP); break; }
if (q==0) {
// goto again; // <-- this works fine, but why
// do I needed this
MessageBox(0,"read","q==0",MB_ICONSTOP);
// should never happen, IMHO
break; }
}
q--; if (q<0) break;
}
LeaveCriticalSection(&cs);
MessageBox(0,"read","finished",MB_ICONINFORMATION);
return 0;
}
int main()
{
InitializeCriticalSection(&cs);
ev = CreateEvent(0,FALSE,FALSE,0);
if (0==ev) { return -1; }
HANDLE t[2];
t[0] = CreateThread(0,0,stWriteThread,0,0,0);
t[1] = CreateThread(0,0,stReadThread,0,0,0);
WaitForMultipleObjects(2,t,TRUE,INFINITE);
return 0;
}
Lucian Wischik
>But, for some reasons, WFSO returns WAIT_OBJECT_O even
>if q is still 0, what IMHO should never happen in the following code.
The reader thread decrements q, and it reaches 0.
The writer thread reaches its end, and calls the final SetEvent.
The reader thread sees that q==0, and waitsforevent. But the event has
already been set, so it just continues. With q==0.
Since I answered your question, I get the priviledge of moralising:
(1) If you'd put in debug statements into your code, you'd have seen
the error. Moreover, these debug statements wouldn't have compromised
the nondeterminism of your code, since everything was serialised by
the critical section.
(2) The code's ugly! Unreadable! I know that thread constructs are an
unpleasant way of writing concurrency, but they can be used more
elegantly than what you've done. Don't use your CriticalSection trick,
of assuming it's acquired and then releasing it at particular moments,
especially not if those moments bridge a "for" construct. It's
unreadable. Discipline yourself to only use critical sections as
though they were part of the hierarchical syntax of C++, i.e.
SYNCHRONISED(&cs) {statements;}
where interleaving them with for/break/while would be simple
impossible.
(Maybe you're used to unix-style conditional-variables or "signals",
where the hold-with-a-brief-moment-of-release is the standard
paradigm. But it's not the standard paradigm for windows-style
events.)
--
Lucian
david lindauer
fast and hit the cleanup at the end, at which time the event gets set without
updating q.
(Sleep takes an arg in milliseconds)
David
Michael Bruschkewitz
lu...@wischik.com says...
> Michael Bruschkewitz <bru...@gmx.net> wrote:
> >But, for some reasons, WFSO returns WAIT_OBJECT_O even
> >if q is still 0, what IMHO should never happen in the following code.
> >Can somebody explain this behaviour?
>
> The reader thread decrements q, and it reaches 0.
> The writer thread reaches its end, and calls the final SetEvent.
From these lines, it is easy to say how long the writer thread runs:
for(int i=0; i<100; ++i) {
Sleep(rand()%256); int n = rand()%8;
statements inside the loop, but this time is marginalized.
For seeing when the writer thread ends, wait for the MsgBox.
Maybe it would be possible to move this line up, so it is possible to
delay the last Event in the writer thread
(I now see that I lost q=-1; in the post, so I add this which don't
change the behaviour.:
MessageBox(0,"write","finished",MB_ICONINFORMATION);
q=-1; SetEvent(ev);
LeaveCriticalSection(&cs);
> The reader thread sees that q==0, and waitsforevent. But the event has
> already been set, so it just continues. With q==0.
the loops are done, after about 12 sec. The q==0 thing happens near
immediately. If I do some output inside the CS's, this happens after
about 10 loops.
> Since I answered your question, I get the priviledge of moralising:
>
> (1) If you'd put in debug statements into your code, you'd have seen
> the error. Moreover, these debug statements wouldn't have compromised
> the nondeterminism of your code, since everything was serialised by
> the critical section.
I just removed all my debug statements because I wanted to show the
problem w/o tapestry.
> (2) The code's ugly! Unreadable! I know that thread constructs are an
> unpleasant way of writing concurrency, but they can be used more
> elegantly than what you've done. Don't use your CriticalSection trick,
> of assuming it's acquired and then releasing it at particular moments,
> especially not if those moments bridge a "for" construct. It's
> unreadable. Discipline yourself to only use critical sections as
> though they were part of the hierarchical syntax of C++, i.e.
> SYNCHRONISED(&cs) {statements;}
> where interleaving them with for/break/while would be simple
> impossible.
I would be able to tune up the code until it would fit into the coding
requirements of life critical coding processes (because this is what
I've done for years), but then, it would need some 100 lines, I suggest.
The whole code is an extract from some sort of thread-safe queue, w/o
the queue. Instead, in the code q is representing the no of elems in the
queue. Therefore, for a TS-Queue a semaphore would be a better
representation, but the CS should be faster.
Regards,
Michael B.
Michael Bruschkewitz
is.net says...
> well the obvious thing is that the write thread is going to complete pretty
> fast and hit the cleanup at the end, at which time the event gets set without
> updating q.
> (Sleep takes an arg in milliseconds)
immediately. Further, I've lost in the posting q=-1 just before the last
SetEvent, but this doesn't change the behaviour. See other post for
details.
Regards,
Michael B.
Michael Bruschkewitz
> fast and hit the cleanup at the end, at which time the event gets set without
> updating q.
> (Sleep takes an arg in milliseconds)
The writer thread needs about 12 sec while the q==0 thing happens near
immediately. Further, I've lost in the posting q=-1 just before the last
SetEvent, but this doesn't change the behaviour. See other post for
details.
I can imagine the Win32-environment releases the waiting thread, and
then resets the event, and in the time between the reader thread has
already made the complete loop and then enters again WFSO where the
event is still set. But this happens too if I use a manual-reset event
and do the ResetEvent() just before --q, for example. This would imply
there the event-functionality is non-deterministic, what's annoying to
me.
Regards,
Michael B.
Michael Bruschkewitz
I finally found the bug.
q was touching 0, writer thread increased q and set event, reader
checked for q==0 and decided not to wait, but later, after counting down
q to 0 again, waited and then the event was received.
Following lines in reader solved the problem:
for(;;) {
bool wait(q==0);
LeaveCriticalSection(&cs); EnterCriticalSection(&cs);
if (wait) {
Sh* happens.
Regards,
Michael B.
--
It's not a bag, it's a future.
Lucian Wischik
>I'm in doubt you have compiled and executed the code.
>From these lines, it is easy to say how long the writer thread runs:
> for(int i=0; i<100; ++i) {
> Sleep(rand()%256); int n = rand()%8;
>This will need about 12800 ms.
Indeed, I did compile and execute the code, and I did observe the
error message after it had run for that long.
I don't know why you're being so defensive. I correctly identified the
error in your code, that explained why you were getting the behaviour
that you questioned.
>This, indeed, could be happen because I lost this q=-1. But, not before
>the loops are done, after about 12 sec. The q==0 thing happens near
>immediately. If I do some output inside the CS's, this happens after
>about 10 loops.
I suggest that a random run had caused it to take less time. In the
absence of debugging, you appear to have no way of knowing where the
error occured. When I added debugging, I saw that the error occured
always at the end of the writer thread, and never before.
--
Lucian
Michael Bruschkewitz
lu...@wischik.com says...
> Michael Bruschkewitz <bru...@gmx.net> wrote:
> >I'm in doubt you have compiled and executed the code.
> >From these lines, it is easy to say how long the writer thread runs:
> > for(int i=0; i<100; ++i) {
> > Sleep(rand()%256); int n = rand()%8;
> >This will need about 12800 ms.
>
> Indeed, I did compile and execute the code, and I did observe the
> error message after it had run for that long.
W2P and WNT, and the error occurs deterministic at the same point, w/
debugging vars after 13 Events, w/ debugging output similar. But it
always happend deterministic and _never_ reached the end, except when I
removed the first Leave/EnterCS-Line in the reader. This leads to the
real cause of the problem, but I haven't seen this before OP.
> I suggest that a random run had caused it to take less time. In the
> absence of debugging, you appear to have no way of knowing where the
> error occured. When I added debugging, I saw that the error occured
> always at the end of the writer thread, and never before.
As I already mentioned, I've made a lot of debugging before bugging the
NG. Unfortunately, multi-thread debugging is not very comfortable in
Visual-C++. (Or I'm not familar with it.)
I did not use srand() to get deterministic behaviour, what indeed
happens.
Maybe your debugging code changed the sequence of statements from my
code, or something else. Truly, if I'd changed the code as you
suggested, this would have solved the problem and then the final
SetEvent would have caused the error because of the lost "q=-1". But my
problem was: why caused the posted code the error?
The problem is solved now and I don't want to argue.
I didn't want to be rude, but because I'm not a native english speaker I
always prefer to be clear than kind. Sorry if I've hurd some way.
Regards,
Michael B.