[RFA/JVMTI] Better env list locking

[Keith Seitz <keiths at redhat dot com>]

Hi,

A long time ago, I noted errors in the environment list locking in 
jvmti.cc. Well, at long last, the time has come to fix it. With the 
recent merge, we now have access to ReentrantReadWriteLock, which is 
what is now used to lock the environment list.

Once again, this patch looks a lot bigger than it is. Much of it just 
moves some chunks of code around.

Comments/questions/concerns?
Keith

ChangeLog
2007-02-01  Keith Seitz  <keiths@redhat.com>

        * jvmti.cc (_envListLock): Change type to
        ReentrantReadWriteLock.
        (_Jv_JVMTI_DisposeEnvironment): Switch to read/write
        lock.
        (check_enabled_event): Likewise.
        (_Jv_GetJVMTIEnv): Likewise.
        (_Jv_JVMTI_Init): Likewise.
        (_Jv_JVMTI_PostEvent): Likewise.

Index: jvmti.cc
===================================================================
--- jvmti.cc	(revision 121442)
+++ jvmti.cc	(working copy)
@@ -27,7 +27,6 @@
 
 #include <java/lang/Class.h>
 #include <java/lang/ClassLoader.h>
-#include <java/lang/Object.h>
 #include <java/lang/OutOfMemoryError.h>
 #include <java/lang/Thread.h>
 #include <java/lang/ThreadGroup.h>
@@ -37,6 +36,8 @@
 #include <java/lang/reflect/Modifier.h>
 #include <java/util/Collection.h>
 #include <java/util/HashMap.h>
+#include <java/util/concurrent/locks/Lock.h>
+#include <java/util/concurrent/locks/ReentrantReadWriteLock.h>
 #include <java/net/URL.h>
 
 static void check_enabled_events (void);
@@ -103,7 +104,8 @@
   struct jvmti_env_list *next;
 };
 static struct jvmti_env_list *_jvmtiEnvironments = NULL;
-static java::lang::Object *_envListLock = NULL;
+static java::util::concurrent::locks::
+ReentrantReadWriteLock *_envListLock = NULL;
 #define FOREACH_ENVIRONMENT(Ele) \
   for (Ele = _jvmtiEnvironments; Ele != NULL; Ele = Ele->next)
 
@@ -896,7 +898,7 @@
     return JVMTI_ERROR_INVALID_ENVIRONMENT;
   else
     {
-      JvSynchronize dummy (_envListLock);
+      _envListLock->writeLock ()->lock ();
       if (_jvmtiEnvironments->env == env)
 	{
 	  struct jvmti_env_list *next = _jvmtiEnvironments->next;
@@ -909,12 +911,16 @@
 	  while (e->next != NULL && e->next->env != env)
 	    e = e->next;
 	  if (e->next == NULL)
-	    return JVMTI_ERROR_INVALID_ENVIRONMENT;
+	    {
+	      _envListLock->writeLock ()->unlock ();
+	      return JVMTI_ERROR_INVALID_ENVIRONMENT;
+	    }
 
 	  struct jvmti_env_list *next = e->next->next;
 	  _Jv_Free (e->next);
 	  e->next = next;
 	}
+      _envListLock->writeLock ()->unlock ();
     }
 
   _Jv_Free (env);
@@ -1215,18 +1221,24 @@
 
   int index = EVENT_INDEX (type); // safe since caller checks this
 
-  JvSynchronize dummy (_envListLock);
-  struct jvmti_env_list *e;
-  FOREACH_ENVIRONMENT (e)
+  if (_jvmtiEnvironments != NULL)
     {
-      char *addr
-	= reinterpret_cast<char *> (&e->env->callbacks) + offset;
-      void **callback = reinterpret_cast<void **> (addr);
-      if (e->env->enabled[index] && *callback != NULL)
+      _envListLock->readLock ()->lock ();
+      struct jvmti_env_list *e;
+      FOREACH_ENVIRONMENT (e)
 	{
-	  *enabled = true;
-	  return;
+	  char *addr
+	    = reinterpret_cast<char *> (&e->env->callbacks) + offset;
+	  void **callback = reinterpret_cast<void **> (addr);
+	  if (e->env->enabled[index] && *callback != NULL)
+	    {
+	      *enabled = true;
+	      _envListLock->readLock ()->unlock ();
+	      return;
+	    }
 	}
+
+      _envListLock->readLock ()->unlock ();
     }
 
   *enabled = false;
@@ -1739,25 +1751,23 @@
   _Jv_JVMTIEnv *env
     = (_Jv_JVMTIEnv *) _Jv_MallocUnchecked (sizeof (_Jv_JVMTIEnv));
   env->p = &_Jv_JVMTI_Interface;
+  struct jvmti_env_list *element
+    = (struct jvmti_env_list *) _Jv_MallocUnchecked (sizeof (struct jvmti_env_list));
+  element->env = env;
+  element->next = NULL;
 
-  {
-    JvSynchronize dummy (_envListLock);
-    struct jvmti_env_list *element
-      = (struct jvmti_env_list *) _Jv_MallocUnchecked (sizeof (struct jvmti_env_list));
-    element->env = env;
-    element->next = NULL;
+  _envListLock->writeLock ()->lock ();
+  if (_jvmtiEnvironments == NULL)
+    _jvmtiEnvironments = element;
+  else
+    {
+      struct jvmti_env_list *e;
+      for (e = _jvmtiEnvironments; e->next != NULL; e = e->next)
+	;
+      e->next = element;
+    }
+  _envListLock->writeLock ()->unlock ();
 
-    if (_jvmtiEnvironments == NULL)
-      _jvmtiEnvironments = element;
-    else
-      {
-	struct jvmti_env_list *e;
-	for (e = _jvmtiEnvironments; e->next != NULL; e = e->next)
-	  ;
-	e->next = element;
-      }
-  }
-
   /* Mark JVMTI active. This is used to force the interpreter
      to use either debugging or non-debugging code. Once JVMTI
      has been enabled, the non-debug interpreter cannot be used. */
@@ -1769,7 +1779,8 @@
 _Jv_JVMTI_Init ()
 {
   _jvmtiEnvironments = NULL;
-  _envListLock = new java::lang::Object ();
+  _envListLock
+    = new java::util::concurrent::locks::ReentrantReadWriteLock ();
 
   // No environments, so this should set all JVMTI:: members to false
   check_enabled_events ();
@@ -2133,7 +2144,7 @@
   va_list args;
   va_start (args, event_thread);
 
-  JvSynchronize dummy (_envListLock);
+  _envListLock->readLock ()->lock ();
   struct jvmti_env_list *e;
   FOREACH_ENVIRONMENT (e)
     {
@@ -2149,6 +2160,6 @@
 	  post_event (e->env, type, event_thread, args);
 	}
     }
-
+  _envListLock->readLock ()->unlock ();
   va_end (args);
 }