[win32] notifyAll also notifying following wait calls

[win32] notifyAll also notifying following wait calls

[Marco Trudel]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

Hey list

win32-threads.cc has a thread race for notifyAll() and wait(). Test.java 
should actually deadlock, but sometimes it doesn't because the second 
wait increments blocked_count before the event is reset and thus will 
also be called.
My proposed patch moves the reset of the event into notifyAll because 
this way, the even is reset just after all waiting threads have been 
notified instead of when the last waiting thread has started working 
again. As I said, there might be a new thread which called wait() before 
the last waiting thread would reset the event.

Was I more or less clear? I'd really appreciate if a couple of persons 
could take a look at the problem and my proposed solution because it's 
always a good thing to triple check concurrency problems...


thanks
Marco

[-- Attachment #2: Test.java --]
[-- Type: text/plain, Size: 841 bytes --]

public class Test
{
	private static final Object mainLock = new Object();

	public static void main(String[] args) throws Exception
	{
		new Thread()
		{
			public void run()
			{
				synchronized(mainLock)
				{
					try
					{
						System.out.println("wait 1 start");
						mainLock.wait(); // might be notified
						System.out.println("wait 1 done");
					} catch (InterruptedException e)
					{
						e.printStackTrace();
					}
				}
			}
		}.start();

		new Thread()
		{
			public void run()
			{
				synchronized(mainLock)
				{
					mainLock.notifyAll();

					try
					{
						System.out.println("wait 2 start");
						mainLock.wait(); // not allowed to be notified 
						System.out.println("wait 2 done! Not allowed!");
					} catch (InterruptedException e)
					{
						e.printStackTrace();
					}
				}
			}
		}.start();
	}
}

[-- Attachment #3: win32-threads.txt --]
[-- Type: text/plain, Size: 964 bytes --]

Index: win32-threads.cc
===================================================================
--- win32-threads.cc	(revision 121541)
+++ win32-threads.cc	(working copy)
@@ -177,14 +177,8 @@
 
   EnterCriticalSection(&cv->count_mutex);
   cv->blocked_count--;
-  // If we were unblocked by the second event (the broadcast one)
-  // and nobody is left, then reset the event.
-  int last_waiter = (rval == (WAIT_OBJECT_0 + 1)) && (cv->blocked_count == 0);
   LeaveCriticalSection(&cv->count_mutex);
 
-  if (last_waiter)
-    ResetEvent (cv->ev[1]);
-
   // Call _Jv_MutexLock repeatedly until the mutex's refcount is the
   // same as before we originally released it.
   while (curcount < count)
@@ -248,7 +242,10 @@
   LeaveCriticalSection (&cv->count_mutex);
 
   if (somebody_is_blocked)
-    SetEvent (cv->ev[1]);
+    {
+      SetEvent (cv->ev[1]); // notify all waiting threads
+      ResetEvent (cv->ev[1]); // reset the notification
+    }
 
   return 0;
 }

Re: [win32] notifyAll also notifying following wait calls

[Tom Tromey]

>>>>> "Marco" == Marco Trudel <mtrudel@gmx.ch> writes:

Marco> win32-threads.cc has a thread race for notifyAll() and
Marco> wait(). Test.java should actually deadlock, but sometimes it
Marco> doesn't because the second wait increments blocked_count before
Marco> the event is reset and thus will also be called.

I can't really comment on your patch, since I don't know much about
Windows.

But note that Object.wait() can wake up spuriously.  So...

Marco>             mainLock.wait(); // not allowed to be notified 

... I think this comment is incorrect.

Tom

Re: [win32] notifyAll also notifying following wait calls

[Marco Trudel]

Tom Tromey wrote:
>>>>>> "Marco" == Marco Trudel <mtrudel@gmx.ch> writes:
> 
> Marco> win32-threads.cc has a thread race for notifyAll() and
> Marco> wait(). Test.java should actually deadlock, but sometimes it
> Marco> doesn't because the second wait increments blocked_count before
> Marco> the event is reset and thus will also be called.
> 
> I can't really comment on your patch, since I don't know much about
> Windows.

Ok, I'll work with it some time and if I don't run into trouble, I'll 
repost the patch again.

> But note that Object.wait() can wake up spuriously.  So...

How so? Wouldn't that make the whole wait() notify() mechanism pretty 
useless?

> Marco>             mainLock.wait(); // not allowed to be notified 
> 
> ... I think this comment is incorrect.

It is allowed to be notified from the preceding notifyAll() call?


thanks
Marco

Re: [win32] notifyAll also notifying following wait calls

[Tom Tromey]

>>>>> "Marco" == Marco Trudel <mtrudel@gmx.ch> writes:

>> But note that Object.wait() can wake up spuriously.  So...

Marco> How so? Wouldn't that make the whole wait() notify() mechanism pretty
Marco> useless?

I'm just saying, Object.wait() is allowed to wake up spuriously.  I
haven't looked deeply into why it might be defined this way.  It
definitely isn't useless, it is just that the way to use it is a bit
different from what one might first expect.

Marco> mainLock.wait(); // not allowed to be notified
>> ... I think this comment is incorrect.

Marco> It is allowed to be notified from the preceding notifyAll() call?

I didn't read that closely, since it can wake up for no reason at all.
User code has to expect this, which is why the proper way to use
wait() is to call it in a loop and then check some external condition
before exiting the loop.

Tom

RE: [win32] notifyAll also notifying following wait calls

[Boehm, Hans]

> -----Original Message-----
> From:  Tom Tromey
> Marco> [Object.wait()] is allowed to be notified from the preceding 
> notifyAll() call?
> 
> I didn't read that closely, since it can wake up for no reason at all.
> User code has to expect this, which is why the proper way to use
> wait() is to call it in a loop and then check some external 
> condition before exiting the loop.
> 
In the vast majority of cases, this is true anyway, since some other
threads could have come in between notify/notifyAll and the wait,
invalidating the condition that was signalled by the notify.  One
argument for allowing spurious wakeups is that it less likely that
people will erroneously omit the loop.  And conversely, given that the
loop has to be there anyway, there's no good reason to have the runtime
work really hard to avoid spurious wakeups.  (Of course, a lot of
spurious wakeups turn into a performance problem.)

Hans