Improve MS-Windows implementation of threads.

 src/systhread.c (sys_cond_init): Set the 'initialized' member to
 true only if initialization is successful.  Initialize wait_count
 and wait_count_lock.
 (sys_cond_wait, sys_cond_signal, sys_cond_broadcast): If
 'initialized' is false, do nothing.
 (sys_cond_wait): Fix the implementation to avoid the "missed
 wakeup" bug: count the waiting threads, and reset the broadcast
 event once the last thread was released.
 (sys_cond_signal, sys_cond_broadcast): Use SetEvent instead of
 PulseEvent.  Don't signal the event if no threads are waiting.
 (sys_cond_destroy): Only close non-NULL handles.
 (sys_thread_create): Return zero if unsuccessful, 1 if successful.
 src/systhread.h (w32thread_cond_t): New member 'initialized'.
 Rename waiters_count and waiters_count_lock to wait_count and
 wait_count_lock, respectively.

diff --git a/src/ChangeLog b/src/ChangeLog
index 0aef16d34e3c463933e17359ca856bf1b2491e03..3e901d84db9374f20a7bd351436ffc87ae52e481 100644 (file)
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,22 @@
+2013-08-31  Eli Zaretskii  <eliz@gnu.org>
+
+       * systhread.c (sys_cond_init): Set the 'initialized' member to
+       true only if initialization is successful.  Initialize wait_count
+       and wait_count_lock.
+       (sys_cond_wait, sys_cond_signal, sys_cond_broadcast): If
+       'initialized' is false, do nothing.
+       (sys_cond_wait): Fix the implementation to avoid the "missed
+       wakeup" bug: count the waiting threads, and reset the broadcast
+       event once the last thread was released.
+       (sys_cond_signal, sys_cond_broadcast): Use SetEvent instead of
+       PulseEvent.  Don't signal the event if no threads are waiting.
+       (sys_cond_destroy): Only close non-NULL handles.
+       (sys_thread_create): Return zero if unsuccessful, 1 if successful.
+
+       * systhread.h (w32thread_cond_t): New member 'initialized'.
+       Rename waiters_count and waiters_count_lock to wait_count and
+       wait_count_lock, respectively.
+
 2013-08-30  Eli Zaretskii  <eliz@gnu.org>
 
        * systhread.h (w32thread_critsect, w32thread_cond_t, sys_mutex_t)

diff --git a/src/process.c b/src/process.c
index 94ca3d4b1a0ea038c2d1a6744cd3c59ae6397313..899c00358662ce5a617e0746204b9346264402e5 100644 (file)
--- a/src/process.c
+++ b/src/process.c
@@ -497,7 +497,6 @@ void
 delete_read_fd (int fd)
 {
   eassert (fd < MAXDESC);
-  eassert (fd <= max_desc);
   delete_keyboard_wait_descriptor (fd);
 
   if (fd_callback_info[fd].flags == 0)
@@ -559,7 +558,6 @@ delete_write_fd (int fd)
   int lim = max_desc;
 
   eassert (fd < MAXDESC);
-  eassert (fd <= max_desc);
 
 #ifdef NON_BLOCKING_CONNECT
   if ((fd_callback_info[fd].flags & NON_BLOCKING_CONNECT_FD) != 0)
@@ -6942,7 +6940,6 @@ delete_keyboard_wait_descriptor (int desc)
   int lim = max_desc;
 
   eassert (desc >= 0 && desc < MAXDESC);
-  eassert (desc <= max_desc);
 
   fd_callback_info[desc].flags &= ~(FOR_READ | KEYBOARD_FD | PROCESS_FD);
 

diff --git a/src/systhread.c b/src/systhread.c
index b154abaecd6ba4b894179fa5fd42f0acba7fa0ff..bde0f027e78dc3466a87998297c68d6f644d5275 100644 (file)
--- a/src/systhread.c
+++ b/src/systhread.c
@@ -243,37 +243,101 @@ sys_mutex_destroy (sys_mutex_t *mutex)
 void
 sys_cond_init (sys_cond_t *cond)
 {
+  cond->initialized = false;
+  cond->wait_count = 0;
+  /* Auto-reset event for signal.  */
   cond->events[CONDV_SIGNAL] = CreateEvent (NULL, FALSE, FALSE, NULL);
+  /* Manual-reset event for broadcast.  */
   cond->events[CONDV_BROADCAST] = CreateEvent (NULL, TRUE, FALSE, NULL);
+  if (!cond->events[CONDV_SIGNAL] || !cond->events[CONDV_BROADCAST])
+    return;
+  InitializeCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+  cond->initialized = true;
 }
 
 void
 sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex)
 {
-  /* FIXME: This implementation is simple, but incorrect.  Stay tuned
-     for better and more complicated implementation.  */
+  DWORD wait_result;
+  bool last_thread_waiting;
+
+  if (!cond->initialized)
+    return;
+
+  /* Increment the wait count avoiding race conditions.  */
+  EnterCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+  cond->wait_count++;
+  LeaveCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+
+  /* Release the mutex and wait for either the signal or the broadcast
+     event.  */
   LeaveCriticalSection ((LPCRITICAL_SECTION)mutex);
-  WaitForMultipleObjects (2, cond->events, FALSE, INFINITE);
+  wait_result = WaitForMultipleObjects (2, cond->events, FALSE, INFINITE);
+
+  /* Decrement the wait count and see if we are the last thread
+     waiting on the condition variable.  */
+  EnterCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+  cond->wait_count--;
+  last_thread_waiting =
+    wait_result == WAIT_OBJECT_0 + CONDV_BROADCAST
+    && cond->wait_count == 0;
+  LeaveCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+
+  /* Broadcast uses a manual-reset event, so when the last thread is
+     released, we must manually reset that event.  */
+  if (last_thread_waiting)
+    ResetEvent (cond->events[CONDV_BROADCAST]);
+
+  /* Per the API, re-acquire the mutex.  */
   EnterCriticalSection ((LPCRITICAL_SECTION)mutex);
 }
 
 void
 sys_cond_signal (sys_cond_t *cond)
 {
-  PulseEvent (cond->events[CONDV_SIGNAL]);
+  bool threads_waiting;
+
+  if (!cond->initialized)
+    return;
+
+  EnterCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+  threads_waiting = cond->wait_count > 0;
+  LeaveCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+
+  if (threads_waiting)
+    SetEvent (cond->events[CONDV_SIGNAL]);
 }
 
 void
 sys_cond_broadcast (sys_cond_t *cond)
 {
-  PulseEvent (cond->events[CONDV_BROADCAST]);
+  bool threads_waiting;
+
+  if (!cond->initialized)
+    return;
+
+  EnterCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+  threads_waiting = cond->wait_count > 0;
+  LeaveCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
+
+  if (threads_waiting)
+    SetEvent (cond->events[CONDV_BROADCAST]);
 }
 
 void
 sys_cond_destroy (sys_cond_t *cond)
 {
-  CloseHandle (cond->events[CONDV_SIGNAL]);
-  CloseHandle (cond->events[CONDV_BROADCAST]);
+  if (cond->events[CONDV_SIGNAL])
+    CloseHandle (cond->events[CONDV_SIGNAL]);
+  if (cond->events[CONDV_BROADCAST])
+    CloseHandle (cond->events[CONDV_BROADCAST]);
+
+  if (!cond->initialized)
+    return;
+
+  /* FIXME: What if wait_count is non-zero, i.e. there are still
+     threads waiting on this condition variable?  */
+  DeleteCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
 }
 
 sys_thread_t
@@ -322,7 +386,7 @@ sys_thread_create (sys_thread_t *thread_ptr, const char *name,
      rule in many places...  */
   thandle = _beginthread (w32_beginthread_wrapper, stack_size, arg);
   if (thandle == (uintptr_t)-1L)
-    return errno;
+    return 0;
 
   /* Kludge alert!  We use the Windows thread ID, an unsigned 32-bit
      number, as the sys_thread_t type, because that ID is the only
@@ -337,7 +401,7 @@ sys_thread_create (sys_thread_t *thread_ptr, const char *name,
      Therefore, we return some more or less arbitrary value of the
      thread ID from this function. */
   *thread_ptr = thandle & 0xFFFFFFFF;
-  return 0;
+  return 1;
 }
 
 void

diff --git a/src/systhread.h b/src/systhread.h
index 52735449a5ef086209e53491eadb5a4e5305e68f..b38fd8ffd452fa029326486eccaa7510b52f6b50 100644 (file)
--- a/src/systhread.h
+++ b/src/systhread.h
@@ -56,9 +56,13 @@ typedef struct {
 enum { CONDV_SIGNAL = 0, CONDV_BROADCAST = 1, CONDV_MAX = 2 };
 
 typedef struct {
-  unsigned waiters_count;
-  w32thread_critsect waiters_count_lock;
+  /* Count of threads that are waiting for this condition variable.  */
+  unsigned wait_count;
+  /* Critical section to protect changes to the count above.  */
+  w32thread_critsect wait_count_lock;
+  /* Handles of events used for signal and broadcast.  */
   void *events[CONDV_MAX];
+  bool initialized;
 } w32thread_cond_t;
 
 typedef w32thread_critsect sys_mutex_t;