From e57df8f77901a3964d21c3d57fb6769cf4511dc2 Mon Sep 17 00:00:00 2001 From: Eli Zaretskii Date: Sat, 31 Aug 2013 14:29:05 +0300 Subject: [PATCH] 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. --- src/ChangeLog | 19 ++++++++++++ src/process.c | 3 -- src/systhread.c | 82 +++++++++++++++++++++++++++++++++++++++++++------ src/systhread.h | 8 +++-- 4 files changed, 98 insertions(+), 14 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 0aef16d34e3..3e901d84db9 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,22 @@ +2013-08-31 Eli Zaretskii + + * 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 * systhread.h (w32thread_critsect, w32thread_cond_t, sys_mutex_t) diff --git a/src/process.c b/src/process.c index 94ca3d4b1a0..899c0035866 100644 --- 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 b154abaecd6..bde0f027e78 100644 --- 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 52735449a5e..b38fd8ffd45 100644 --- 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; -- 2.39.5