Re: Patch for Review: PR/9254 and other Win32 Threading Fixes

[Ranjit Mathew <rmathew at hotmail dot com>]

Ranjit Mathew wrote:
> > Doesn't this reset the owner field to zero even for a nested 
> > _Jv_MutexUnlock call?  I suspect you need a count field to correctly 
> > maintain the owner field, unless you can somehow get at the one that's 
> > already inside the CriticalSection.
>
> Your observation about the potential bug is correct.
> Sorry! (And Tom would also chide me for not including a
> "Fixes PR java/9254" in the ChangeLog - Sorry again!)
Submitting this (yet) again after incorporating Hans's feedback:

--------------------------------- 8< ---------------------------------
2003-01-28  Ranjit Mathew  <rmathew@hotmail.com>

          Fixes PR java/9254
          * include/win32-threads.h (_Jv_Mutex_t): Convert to a struct
          additionally containing id of the owner thread as well as
          the number of nested times the thread has acquired the mutex.
          (_Jv_MutexInit): Initialise owner thread id and refcount to 0.
          (_Jv_MutexDestroy): Reset owner thread id and refcount to 0.
          (_Jv_MutexUnlock): Check if really the owner thread, reset
          owner thread id to 0 before leaving, if leaving for the last
          time.
          (_Jv_MutexLock): Set owner thread id in the mutex and increment
          refcount.
          (_Jv_ThreadYield): Yield using a call to Sleep(0).

          * win32-threads.cc (_Jv_CondWait): Check if really owner of
          the passed mutex.
          Pass handle of the 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.


--- include/win32-threads.h	2003-01-14 20:23:56.000000000 +0530
+++ include/win32-threads.h	2003-01-28 19:20:51.000000000 +0530
@@ -2,5 +2,6 @@
 // win32-threads.h - Defines for using Win32 threads.

-/* Copyright (C) 1998, 1999, 2000  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003 Free Software
+   Foundation

    This file is part of libgcj.
@@ -19,11 +20,30 @@
 //

-typedef struct _Jv_ConditionVariable_t {
+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];
-  CRITICAL_SECTION count_mutex;
+
+  // Number of threads waiting on this condition variable
   int blocked_count;
-};

-typedef CRITICAL_SECTION _Jv_Mutex_t;
+  // 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;
+
+  // Track nested mutex acquisitions by the same thread
+  int refcount;
+
+  // The actual Windows construct used to implement this mutex
+  CRITICAL_SECTION cs;
+
+} _Jv_Mutex_t;

 typedef struct
@@ -61,5 +81,7 @@
 inline void _Jv_MutexInit (_Jv_Mutex_t *mu)
 {
-  InitializeCriticalSection(mu);
+  mu->owner = 0UL;
+  mu->refcount = 0;
+  InitializeCriticalSection (&(mu->cs));
 }

@@ -67,5 +89,7 @@
 inline void _Jv_MutexDestroy (_Jv_Mutex_t *mu)
 {
-  DeleteCriticalSection(mu);
+  mu->owner = 0UL;
+  mu->refcount = 0;
+  DeleteCriticalSection (&(mu->cs));
   mu = NULL;
 }
@@ -73,11 +97,21 @@
 inline int _Jv_MutexUnlock (_Jv_Mutex_t *mu)
 {
-  LeaveCriticalSection(mu);
-  return 0;
+  if (mu->owner == GetCurrentThreadId ( ))
+    {
+      mu->refcount--;
+      if (mu->refcount == 0)
+        mu->owner = 0UL;
+      LeaveCriticalSection (&(mu->cs));
+      return 0;
+    }
+  else
+    return 1;
 }

 inline int _Jv_MutexLock (_Jv_Mutex_t *mu)
 {
-  EnterCriticalSection(mu);
+  EnterCriticalSection (&(mu->cs));
+  mu->owner = GetCurrentThreadId ( );
+  mu->refcount++;
   return 0;
 }
@@ -105,7 +139,5 @@
 inline void _Jv_ThreadYield (void)
 {
-  // FIXME: win98 freezes hard (OS hang) when we use this --
-  //        for now, we simply don't yield
-  // Sleep (0);
+  Sleep (0);
 }

--- win32-threads.cc	2003-01-14 20:23:53.000000000 +0530
+++ win32-threads.cc	2003-01-28 19:21:56.000000000 +0530
@@ -1,5 +1,6 @@
 // win32-threads.cc - interface between libjava and Win32 threads.

-/* Copyright (C) 1998, 1999  Free Software Foundation, Inc.
+/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003 Free Software
+   Foundation, Inc.

    This file is part of libgcj.
@@ -71,10 +72,12 @@
 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");
-  }
+  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");
+    }
 }

@@ -86,9 +89,11 @@
 _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);
+  EnterCriticalSection (&cv->count_mutex);
+  ensure_condvar_initialized (cv);
   cv->blocked_count++;
-  LeaveCriticalSection(&cv->count_mutex);
+  LeaveCriticalSection (&cv->count_mutex);

   DWORD time;
@@ -103,16 +108,15 @@
   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;
+  // 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]);
+  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;
+  return 0;
 }

@@ -122,5 +126,5 @@
   // we do lazy creation of Events since CreateEvent() is insanely expensive
   cv->ev[0] = 0;
-  InitializeCriticalSection(&cv->count_mutex);
+  InitializeCriticalSection (&cv->count_mutex);
   cv->blocked_count = 0;
 }
@@ -129,30 +133,49 @@
 _Jv_CondDestroy (_Jv_ConditionVariable_t *cv)
 {
-  if (cv->ev[0] != 0) CloseHandle(cv->ev[0]);
+  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 *)
+_Jv_CondNotify (_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *mu)
 {
-  EnterCriticalSection(&cv->count_mutex);
-  ensure_condvar_initialized(cv);
+  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);
+  LeaveCriticalSection (&cv->count_mutex);
+
+  if (somebody_is_blocked)
+    SetEvent (cv->ev[0]);

-  if (somebody_is_blocked) return SetEvent (cv->ev[0]) ? 0 : GetLastError();
-  else return 0;
+  return 0;
 }

 int
-_Jv_CondNotifyAll (_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *)
+_Jv_CondNotifyAll (_Jv_ConditionVariable_t *cv, _Jv_Mutex_t *mu)
 {
-  EnterCriticalSection(&cv->count_mutex);
-  ensure_condvar_initialized(cv);
+  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);
+  LeaveCriticalSection (&cv->count_mutex);

-  if (somebody_is_blocked) return SetEvent (cv->ev[1]) ? 0 : GetLastError();
-  else return 0;
+  if (somebody_is_blocked)
+    SetEvent (cv->ev[1]);
+
+  return 0;
 }

@@ -166,6 +189,6 @@
   _Jv_ThreadKey = TlsAlloc();
   _Jv_ThreadDataKey = TlsAlloc();
-  daemon_mutex = CreateMutex(NULL, 0, NULL);
-  daemon_cond = CreateEvent(NULL, 0, 0, NULL);
+  daemon_mutex = CreateMutex (NULL, 0, NULL);
+  daemon_cond = CreateEvent (NULL, 1, 0, NULL);
   non_daemon_count = 0;
 }
@@ -256,5 +279,5 @@
       non_daemon_count--;
       if (! non_daemon_count)
-          PulseEvent (daemon_cond);
+        SetEvent (daemon_cond);
       ReleaseMutex (daemon_mutex);
     }
@@ -298,8 +321,10 @@
 _Jv_ThreadWait (void)
 {
-  WaitForSingleObject(daemon_mutex, INFINITE);
-  if(non_daemon_count)
-      SignalObjectAndWait(daemon_mutex, daemon_cond, INFINITE, 0);
-  ReleaseMutex(daemon_mutex);
+  WaitForSingleObject (daemon_mutex, INFINITE);
+  if (non_daemon_count)
+    {
+      ReleaseMutex (daemon_mutex);
+      WaitForSingleObject (daemon_cond, INFINITE);
+    }
 }

--------------------------------- 8< ---------------------------------

Ranjit.

--
Ranjit Mathew        Email: rmathew AT hotmail DOT com
Bangalore,
INDIA.               Web: http://ranjitmathew.tripod.com/