glib win32 condvar implementation

[Alexander Larsson <alexl redhat com>]

I was looking over the gthread win32 implementation today when doing
some dbus threading work, and i noticed what i think is a bug:

In g_cond_wait_internal() we add the even to the event array in the
condvar, then we sleep on it. When we wake up or timeout we then remove
the event object from the array... Except we don't, we only do that in
the case of a timeout. I really think we always should remove it, and in
fact we should reset it always too, in case we got some extra wakeup
call inbetween.

The attached (not tested) patch gives an idea of what sort of changes i
think are needed.

Am I on crack, or is this totally broken?

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's an old-fashioned bohemian Green Beret with a winning smile and a way with 
the ladies. She's a mentally unstable communist Hell's Angel from a secret 
island of warrior women. They fight crime! 

? a
? glib-base64.patch
? glib-display-filenames.patch
? glib-mem-tracker.patch
? gobject-memuse.patch
? docs/a
? glib/a
? glib/gbase64.S
? glib/gettext-filenames-try1.patch
? glib/gettext-filenames.patch
? glib/gettext-files.patch
? gobject/a
? tests/a
? tests/analyze-memdump
? tests/analyze-memdump.c
? tests/decode-tracking
? tests/nautilus-mem.txt
? tests/nautilus-run-with-home.dat
? tests/nautilus_after.dat
? tests/nautilus_before.dat
? tests/nautilus_layout.dat
? tests/nautilus_layout_topleft.dat
? tests/nautilus_layout_topleft_iconprivate.dat
? tests/test_visible_layouts.dat
? tests/test_w_layouts.dat
? tests/test_wo_layouts.dat
Index: gthread/gthread-win32.c
===================================================================
RCS file: /cvs/gnome/glib/gthread/gthread-win32.c,v
retrieving revision 1.12
diff -u -p -r1.12 gthread-win32.c
--- gthread/gthread-win32.c	11 May 2006 00:08:31 -0000	1.12
+++ gthread/gthread-win32.c	24 Aug 2006 15:57:38 -0000
@@ -246,20 +246,21 @@ g_cond_wait_internal (GCond *cond,
 
   g_thread_functions_for_glib_use_default.mutex_lock (entered_mutex);
 
-  if (retval == WAIT_TIMEOUT)
-    {
-      EnterCriticalSection (&cond->lock);
-      g_ptr_array_remove (cond->array, event);
-
-      /* In the meantime we could have been signaled, so we must again
-       * wait for the signal, this time with no timeout, to reset
-       * it. retval is set again to honour the late arrival of the
-       * signal */
-      win32_check_for_error (WAIT_FAILED !=
-			     (retval = WaitForSingleObject (event, 0)));
+  EnterCriticalSection (&cond->lock);
+  g_ptr_array_remove (cond->array, event);
 
-      LeaveCriticalSection (&cond->lock);
-    }
+  /* In the meantime we could have been signaled. If we got a timeout the
+   * last time we check for it with no timeout to honor the late arrival of the
+   * signal. If we already got a signal we just reset the event to make sure
+   * it doesn't intefer with the next wait. */
+  if (retval == WAIT_TIMEOUT)
+    win32_check_for_error (WAIT_FAILED !=
+			   (retval = WaitForSingleObject (event, 0)));
+  else
+    win32_check_for_error (0 !=
+			   (ResetEvent (event)));
+  
+  LeaveCriticalSection (&cond->lock);
 
 #ifndef G_DISABLE_ASSERT
   EnterCriticalSection (&cond->lock);