Patch for Review: PR/9254 and other Win32 Threading Fixes
Ranjit Mathew
rmathew@hotmail.com
Thu Jan 23 07:39:00 GMT 2003
[NOTE: CC to Adam for review and possible approval. My apologies
if delivered twice.]
Hi,
This patch proposes to clean up the Win32 threads support in
libgcj a bit and contains fixes for a few problems reported
on the main list. These include:
* Erik's notifyAll( ) bug (PR/9254).
* Incorrect return values in _Jv_CondWait, _Jv_Notify and
_Jv_NotifyAll (and even _Jv_MutexUnlock).
* Use of SignalObjectAndWait( ) which is not supported on Win95
and is apparently a dummy on Win98.
* Minor clean up corrections for as yet unborn PRs. ;-)
* Threads do not yield.
* Potential bug with LeaveCriticalSection( ) on an "unowned"
critical section.
* Minor formatting adjustments.
Most of these fixes are rather simple and are explained in more
detail below. If these changes are accepted, we can attempt
implementing interruptible threads on Win32.
Checked and tested on a Win98SE box.
The following are detailed explanations of some of the major changes
in the order in which they appear in the patch:
1. I have had to change _Jv_Mutex_t to a struct containing the
original critical section as well as a DWORD owner thread-id.
This is because it is not possible to otherwise know the owner of
a critical section and releasing an unowned critical section
causes all sorts of arbitrary behaviour (ref. MSDN).
2. The _Jv_MutexInit( ) etc. functions have been changed accordingly.
In particular, the _Jv_MutexUnlock( ) method guards against
releasing unowned critical sections. Note that the critical
section itself is being used to guard access to the thread-id
field.
3. A comment in the _Jv_ThreadYield( ) method says that Sleep(0)
freezes hard on Win98. I have a Win98 box and I have tried
calling Sleep(0) a zillion times in various contexts without
producing a crash. So I uncommented this.
4. _Jv_CondWait( ) now returns the correct values (except for
interrupted status) especially if not owner of the given mutex.
5. The notifyAll( ) bug reported by Erik was due to the fact that
ResetEvent( ) was being passed a *pointer to a handle* instead
of the handle itself - thus the manual-reset event used for
broadcast was never being reset and all subsequent calls to wait( )
immediately succeed.
6. The SignalObjectAndWait( ) function is not supported on Win95
(and is supposed to be a dummy on Win98) and causes even a
"Hello World" program to not run there. This was being used
(and that too, if I'm not mistaken, with incorrect order of
parameters) in _Jv_ThreadWait to wait for all non-daemon
threads to die. I changed the daemon_cond event to a
manual-reset event and changed PulseEvent( ) to SetEvent( )
in really_start( ). Note that the stickiness of such an
event and the use of it for mere signalling purposes avoids
(IMHO) any race-conditions.
ChangeLog:
2003-01-23 Ranjit Mathew <rmathew@hotmail.com>
* include/win32-threads.h (_Jv_Mutex_t): Convert to a struct
containing id of the owner thread as well.
(_Jv_MutexInit): Initialise owner thread id to 0.
(_Jv_MutexDestroy): Reset owner thread id to 0.
(_Jv_MutexUnlock): Check if really the owner thread, reset
owner thread id to 0 before leaving.
(_Jv_MutexLock): Set owner thread id in the mutex.
(_Jv_ThreadYield): Yield using Win32 Sleep(0).
* win32-threads.cc (_Jv_CondWait): Check if really owner of
the passed mutex.
Pass handle of broadcast event, instead of a pointer to it
in Win32 ResetEvent( ) call.
Remove incorrect return values.
(_Jv_CondDestroy): Close both event handles and delete
critical section.
(_Jv_CondNotify): Check if really the owner thread.
(_Jv_CondNotifyAll): Check if really the owner thread.
(_Jv_InitThreads): Change daemon_cond to a manual-reset event.
(really_start): Use SetEvent( ) to signal daemon_cond.
(_Jv_ThreadWait): Remove SignalObjectAndWait( ) and use
WaitForSingleObject( ) instead to wait for daemon_cond to be
signalled.
Patch ("diff -c2p" format):
-------------------------------- 8< --------------------------------
*** include/win32-threads.h Tue Jan 14 20:23:56 2003
--- include/win32-threads.h Thu Jan 23 01:16:26 2003
***************
*** 2,6 ****
// win32-threads.h - Defines for using Win32 threads.
! /* Copyright (C) 1998, 1999, 2000 Free Software Foundation
This file is part of libgcj.
--- 2,7 ----
// win32-threads.h - Defines for using Win32 threads.
! /* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003 Free Software
! Foundation
This file is part of libgcj.
*************** details. */
*** 19,29 ****
//
! typedef struct _Jv_ConditionVariable_t {
HANDLE ev[2];
! CRITICAL_SECTION count_mutex;
int blocked_count;
- };
! typedef CRITICAL_SECTION _Jv_Mutex_t;
typedef struct
--- 20,46 ----
//
! typedef struct
! {
! // ev[0] (signal) is a Win32 auto-reset event for _Jv_CondNotify
! // ev[1] (broadcast) is a Win32 manual-reset event for _Jv_CondNotifyAll
HANDLE ev[2];
!
! // Number of threads waiting on this condition variable
int blocked_count;
! // Protects access to the blocked_count variable
! CRITICAL_SECTION count_mutex;
!
! } _Jv_ConditionVariable_t;
!
! typedef struct
! {
! // The thread-id of the owner thread if any, 0 otherwise
! DWORD owner;
!
! // The actual Windows construct used to implement this mutex
! CRITICAL_SECTION cs;
!
! } _Jv_Mutex_t;
typedef struct
*************** int _Jv_CondNotifyAll (_Jv_ConditionVari
*** 61,65 ****
inline void _Jv_MutexInit (_Jv_Mutex_t *mu)
{
! InitializeCriticalSection(mu);
}
--- 78,83 ----
inline void _Jv_MutexInit (_Jv_Mutex_t *mu)
{
! mu->owner = 0UL;
! InitializeCriticalSection (&(mu->cs));
}
*************** inline void _Jv_MutexInit (_Jv_Mutex_t *
*** 67,71 ****
inline void _Jv_MutexDestroy (_Jv_Mutex_t *mu)
{
! DeleteCriticalSection(mu);
mu = NULL;
}
--- 85,90 ----
inline void _Jv_MutexDestroy (_Jv_Mutex_t *mu)
{
! mu->owner = 0UL;
! DeleteCriticalSection (&(mu->cs));
mu = NULL;
}
*************** inline void _Jv_MutexDestroy (_Jv_Mutex_
*** 73,83 ****
inline int _Jv_MutexUnlock (_Jv_Mutex_t *mu)
{
! LeaveCriticalSection(mu);
! return 0;
}
inline int _Jv_MutexLock (_Jv_Mutex_t *mu)
{
! EnterCriticalSection(mu);
return 0;
}
--- 92,109 ----
inline int _Jv_MutexUnlock (_Jv_Mutex_t *mu)
{
! if (mu->owner == GetCurrentThreadId ( ))
! {
! mu->owner = 0UL;
! LeaveCriticalSection (&(mu->cs));
! return 0;
! }
! else
! return 1;
}
inline int _Jv_MutexLock (_Jv_Mutex_t *mu)
{
! EnterCriticalSection (&(mu->cs));
! mu->owner = GetCurrentThreadId ( );
return 0;
}
*************** inline _Jv_Thread_t *_Jv_ThreadCurrentDa
*** 105,111 ****
inline void _Jv_ThreadYield (void)
{
! // FIXME: win98 freezes hard (OS hang) when we use this --
! // for now, we simply don't yield
! // Sleep (0);
}
--- 131,135 ----
inline void _Jv_ThreadYield (void)
{
! Sleep (0);
}
*** win32-threads.cc Tue Jan 14 20:23:53 2003
--- win32-threads.cc Thu Jan 23 11:45:29 2003
***************
*** 1,5 ****
// win32-threads.cc - interface between libjava and Win32 threads.
! /* Copyright (C) 1998, 1999 Free Software Foundation, Inc.
This file is part of libgcj.
--- 1,6 ----
// win32-threads.cc - interface between libjava and Win32 threads.
! /* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003 Free Software
! Foundation, Inc.
This file is part of libgcj.
*************** inline void
*** 71,80 ****
ensure_condvar_initialized(_Jv_ConditionVariable_t *cv)
{
! if (cv->ev[0] == 0) {
! cv->ev[0] = CreateEvent (NULL, 0, 0, NULL);
! if (cv->ev[0] == 0) JvFail("CreateEvent() failed");
! cv->ev[1] = CreateEvent (NULL, 1, 0, NULL);
! if (cv->ev[1] == 0) JvFail("CreateEvent() failed");
! }
}
--- 72,83 ----
ensure_condvar_initialized(_Jv_ConditionVariable_t *cv)
{
! if (cv->ev[0] == 0)
! {
! cv->ev[0] = CreateEvent (NULL, 0, 0, NULL);
! if (cv->ev[0] == 0) JvFail("CreateEvent() failed");
!
! cv->ev[1] = CreateEvent (NULL, 1, 0, NULL);
! if (cv->ev[1] == 0) JvFail("CreateEvent() failed");
! }
}
*************** int
*** 86,94 ****
_Jv_CondWait(_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *mu, jlong millis, jint nanos)
{
! EnterCriticalSection(&cv->count_mutex);
! ensure_condvar_initialized(cv);
cv->blocked_count++;
! LeaveCriticalSection(&cv->count_mutex);
DWORD time;
--- 89,99 ----
_Jv_CondWait(_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *mu, jlong millis, jint nanos)
{
+ if (mu->owner != GetCurrentThreadId ( ))
+ return _JV_NOT_OWNER;
! EnterCriticalSection (&cv->count_mutex);
! ensure_condvar_initialized (cv);
cv->blocked_count++;
! LeaveCriticalSection (&cv->count_mutex);
DWORD time;
*************** _Jv_CondWait(_Jv_ConditionVariable_t *cv
*** 103,118 ****
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 signal.
! int last_waiter = rval == WAIT_OBJECT_0 + 1 && cv->blocked_count == 0;
LeaveCriticalSection(&cv->count_mutex);
! if (last_waiter) ResetEvent(&cv->ev[1]);
_Jv_MutexLock (mu);
! if (rval == WAIT_FAILED) return GetLastError();
! else if (rval == WAIT_TIMEOUT) return ETIMEDOUT;
! else return 0;
}
--- 108,122 ----
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]);
_Jv_MutexLock (mu);
! return 0;
}
*************** _Jv_CondInit (_Jv_ConditionVariable_t *c
*** 122,126 ****
// we do lazy creation of Events since CreateEvent() is insanely expensive
cv->ev[0] = 0;
! InitializeCriticalSection(&cv->count_mutex);
cv->blocked_count = 0;
}
--- 126,130 ----
// we do lazy creation of Events since CreateEvent() is insanely expensive
cv->ev[0] = 0;
! InitializeCriticalSection (&cv->count_mutex);
cv->blocked_count = 0;
}
*************** void
*** 129,158 ****
_Jv_CondDestroy (_Jv_ConditionVariable_t *cv)
{
! if (cv->ev[0] != 0) CloseHandle(cv->ev[0]);
cv = NULL;
}
int
! _Jv_CondNotify (_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *)
{
! EnterCriticalSection(&cv->count_mutex);
! ensure_condvar_initialized(cv);
int somebody_is_blocked = cv->blocked_count > 0;
! LeaveCriticalSection(&cv->count_mutex);
! if (somebody_is_blocked) return SetEvent (cv->ev[0]) ? 0 : GetLastError();
! else return 0;
}
int
! _Jv_CondNotifyAll (_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *)
{
! EnterCriticalSection(&cv->count_mutex);
! ensure_condvar_initialized(cv);
int somebody_is_blocked = cv->blocked_count > 0;
! LeaveCriticalSection(&cv->count_mutex);
! if (somebody_is_blocked) return SetEvent (cv->ev[1]) ? 0 : GetLastError();
! else return 0;
}
--- 133,181 ----
_Jv_CondDestroy (_Jv_ConditionVariable_t *cv)
{
! if (cv->ev[0] != 0)
! {
! CloseHandle (cv->ev[0]);
! CloseHandle (cv->ev[1]);
!
! cv->ev[0] = 0;
! }
!
! DeleteCriticalSection (&cv->count_mutex);
!
cv = NULL;
}
int
! _Jv_CondNotify (_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *mu)
{
! if (mu->owner != GetCurrentThreadId ( ))
! return _JV_NOT_OWNER;
!
! EnterCriticalSection (&cv->count_mutex);
! ensure_condvar_initialized (cv);
int somebody_is_blocked = cv->blocked_count > 0;
! LeaveCriticalSection (&cv->count_mutex);
!
! if (somebody_is_blocked)
! SetEvent (cv->ev[0]);
! return 0;
}
int
! _Jv_CondNotifyAll (_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *mu)
{
! if (mu->owner != GetCurrentThreadId ( ))
! return _JV_NOT_OWNER;
!
! EnterCriticalSection (&cv->count_mutex);
! ensure_condvar_initialized (cv);
int somebody_is_blocked = cv->blocked_count > 0;
! LeaveCriticalSection (&cv->count_mutex);
! if (somebody_is_blocked)
! SetEvent (cv->ev[1]);
!
! return 0;
}
*************** _Jv_InitThreads (void)
*** 166,171 ****
_Jv_ThreadKey = TlsAlloc();
_Jv_ThreadDataKey = TlsAlloc();
! daemon_mutex = CreateMutex(NULL, 0, NULL);
! daemon_cond = CreateEvent(NULL, 0, 0, NULL);
non_daemon_count = 0;
}
--- 189,194 ----
_Jv_ThreadKey = TlsAlloc();
_Jv_ThreadDataKey = TlsAlloc();
! daemon_mutex = CreateMutex (NULL, 0, NULL);
! daemon_cond = CreateEvent (NULL, 1, 0, NULL);
non_daemon_count = 0;
}
*************** really_start (void* x)
*** 256,260 ****
non_daemon_count--;
if (! non_daemon_count)
! PulseEvent (daemon_cond);
ReleaseMutex (daemon_mutex);
}
--- 279,283 ----
non_daemon_count--;
if (! non_daemon_count)
! SetEvent (daemon_cond);
ReleaseMutex (daemon_mutex);
}
*************** void
*** 298,305 ****
_Jv_ThreadWait (void)
{
! WaitForSingleObject(daemon_mutex, INFINITE);
! if(non_daemon_count)
! SignalObjectAndWait(daemon_mutex, daemon_cond, INFINITE, 0);
! ReleaseMutex(daemon_mutex);
}
--- 321,330 ----
_Jv_ThreadWait (void)
{
! WaitForSingleObject (daemon_mutex, INFINITE);
! if (non_daemon_count)
! {
! ReleaseMutex (daemon_mutex);
! WaitForSingleObject (daemon_cond, INFINITE);
! }
}
-------------------------------- 8< --------------------------------
Sincerely Yours,
Ranjit.
--
Ranjit Mathew Email: rmathew AT hotmail DOT com
Bangalore,
INDIA. Web: http://ranjitmathew.tripod.com/
More information about the Java-patches
mailing list