[RFA/JVMTI] Better env list locking

[RFA/JVMTI] Better env list locking

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

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.

[-- Attachment #2: jvmti-locks.patch --]
[-- Type: text/x-patch, Size: 4626 bytes --]

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);
 }

Re: [RFA/JVMTI] Better env list locking

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

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

Keith> Comments/questions/concerns?

Can post_event or any callback it calls possibly throw an exception?
If so that will leave the lock held.

If not this is ok.  Thanks.

Tom

Re: [RFA/JVMTI] Better env list locking

[Keith Seitz]

Tom Tromey wrote:

> Keith> Comments/questions/concerns?
> 
> Can post_event or any callback it calls possibly throw an exception?
> If so that will leave the lock held.
> 
> If not this is ok.  Thanks.

That's a really good question. The JVMTI specification does talk about 
dealing with exceptions that occur during the event callback, but 
nothing particularly definitive about whether a callback is allowed to 
throw an exception:

"Any exception thrown during the execution of an event callback can 
overwrite any current pending exception in the current application 
thread. Care must be taken to preserve a pending exception when an event 
callback makes a JNI call that might generate an exception."

 From what I read into this, it sounds more like it is up to the agent 
to deal with exceptions properly, not the JVMTI implementation. But then 
perhaps a bit of defensive programming might not hurt? Catch any 
exceptions, release the lock, and rethrow the exception?

I don't really know. I could go either way, really. Your call, Mr. 
Maintainer. :-)

Keith

Re: [RFA/JVMTI] Better env list locking

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith>  From what I read into this, it sounds more like it is up to the agent
Keith> to deal with exceptions properly, not the JVMTI implementation. But
Keith> then perhaps a bit of defensive programming might not hurt? Catch any
Keith> exceptions, release the lock, and rethrow the exception?

My reading is that no exception will be thrown, but rather they'll
just be put into the JNI environment.  This won't affect the locking.

Keith> I don't really know. I could go either way, really. Your call,
Keith> Mr. Maintainer. :-)

Let's just go ahead with the patch as is.
We can always add a try/catch if we find, later, that one is actually
needed.

Tom

Re: [RFA/JVMTI] Better env list locking

[Keith Seitz]

Tom Tromey wrote:

> Keith> I don't really know. I could go either way, really. Your call,
> Keith> Mr. Maintainer. :-)
> 
> Let's just go ahead with the patch as is.
> We can always add a try/catch if we find, later, that one is actually
> needed.

That's a reasonable approach. Committed as-is.

Thank you for reviewing this patch.

Keith

[RFA/JVMTI] Better env list locking

[Keith Seitz]

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.