8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()

Reviewed-by: coleenp, sspitsyn

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

@@ -96,31 +96,69 @@ static inline InstanceKlass* get_ik(jclass def) {
 // If any of the classes are being redefined, wait
 // Parallel constant pool merging leads to indeterminate constant pools.
 void VM_RedefineClasses::lock_classes() {
+  JvmtiThreadState *state = JvmtiThreadState::state_for(JavaThread::current());
+  GrowableArray<Klass*>* redef_classes = state->get_classes_being_redefined();
+
   MonitorLocker ml(RedefineClasses_lock);
+
+  if (redef_classes == NULL) {
+    redef_classes = new(ResourceObj::C_HEAP, mtClass) GrowableArray<Klass*>(1, mtClass);
+    state->set_classes_being_redefined(redef_classes);
+  }
+
   bool has_redefined;
   do {
     has_redefined = false;
-    // Go through classes each time until none are being redefined.
+    // Go through classes each time until none are being redefined. Skip
+    // the ones that are being redefined by this thread currently. Class file
+    // load hook event may trigger new class redefine when we are redefining
+    // a class (after lock_classes()).
     for (int i = 0; i < _class_count; i++) {
-      if (get_ik(_class_defs[i].klass)->is_being_redefined()) {
-        ml.wait();
-        has_redefined = true;
-        break;  // for loop
+      InstanceKlass* ik = get_ik(_class_defs[i].klass);
+      // Check if we are currently redefining the class in this thread already.
+      if (redef_classes->contains(ik)) {
+        assert(ik->is_being_redefined(), "sanity");
+      } else {
+        if (ik->is_being_redefined()) {
+          ml.wait();
+          has_redefined = true;
+          break;  // for loop
+        }
       }
     }
   } while (has_redefined);
+
   for (int i = 0; i < _class_count; i++) {
-    get_ik(_class_defs[i].klass)->set_is_being_redefined(true);
+    InstanceKlass* ik = get_ik(_class_defs[i].klass);
+    redef_classes->push(ik); // Add to the _classes_being_redefined list
+    ik->set_is_being_redefined(true);
   }
   ml.notify_all();
 }
 
 void VM_RedefineClasses::unlock_classes() {
+  JvmtiThreadState *state = JvmtiThreadState::state_for(JavaThread::current());
+  GrowableArray<Klass*>* redef_classes = state->get_classes_being_redefined();
+  assert(redef_classes != NULL, "_classes_being_redefined is not allocated");
+
   MonitorLocker ml(RedefineClasses_lock);
-  for (int i = 0; i < _class_count; i++) {
-    assert(get_ik(_class_defs[i].klass)->is_being_redefined(),
+
+  for (int i = _class_count - 1; i >= 0; i--) {
+    InstanceKlass* def_ik = get_ik(_class_defs[i].klass);
+    if (redef_classes->length() > 0) {
+      // Remove the class from _classes_being_redefined list
+      Klass* k = redef_classes->pop();
+      assert(def_ik == k, "unlocking wrong class");
+    }
+    assert(def_ik->is_being_redefined(),
            "should be being redefined to get here");
-    get_ik(_class_defs[i].klass)->set_is_being_redefined(false);
+
+    // Unlock after we finish all redefines for this class within
+    // the thread. Same class can be pushed to the list multiple
+    // times (not more than once by each recursive redefinition).
+    if (!redef_classes->contains(def_ik)) {
+      def_ik->set_is_being_redefined(false);
+    }
   }
   ml.notify_all();
 }

src/hotspot/share/prims/jvmtiThreadState.cpp

@@ -57,6 +57,7 @@ JvmtiThreadState::JvmtiThreadState(JavaThread* thread)
   _pending_step_for_popframe = false;
   _class_being_redefined = NULL;
   _class_load_kind = jvmti_class_load_kind_load;
+  _classes_being_redefined = NULL;
   _head_env_thread_state = NULL;
   _dynamic_code_event_collector = NULL;
   _vm_object_alloc_event_collector = NULL;
@@ -106,6 +107,10 @@ JvmtiThreadState::JvmtiThreadState(JavaThread* thread)
 JvmtiThreadState::~JvmtiThreadState()   {
   assert(JvmtiThreadState_lock->is_locked(), "sanity check");
 
+  if (_classes_being_redefined != NULL) {
+    delete _classes_being_redefined; // free the GrowableArray on C heap
+  }
+
   // clear this as the state for the thread
   get_thread()->set_jvmti_thread_state(NULL);
 

src/hotspot/share/prims/jvmtiThreadState.hpp

@@ -99,6 +99,7 @@ class JvmtiThreadState : public CHeapObj<mtInternal> {
   // info to the class file load hook event handler.
   Klass*                _class_being_redefined;
   JvmtiClassLoadKind    _class_load_kind;
+  GrowableArray<Klass*>* _classes_being_redefined;
 
   // This is only valid when is_interp_only_mode() returns true
   int               _cur_stack_depth;
@@ -244,6 +245,15 @@ class JvmtiThreadState : public CHeapObj<mtInternal> {
     return _class_load_kind;
   }
 
+  // Get the classes that are currently being redefined by this thread.
+  inline GrowableArray<Klass*>* get_classes_being_redefined() {
+    return _classes_being_redefined;
+  }
+
+  inline void set_classes_being_redefined(GrowableArray<Klass*>* redef_classes) {
+    _classes_being_redefined = redef_classes;
+  }
+
   // RedefineClasses support
   // The bug 6214132 caused the verification to fail.
   //

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TransformerDeadlockTest.java

@@ -0,0 +1,117 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @bug 8241390
+ * @summary Test recursively retransforms the same class. The test hangs if
+ *          a deadlock happens.
+ * @requires vm.jvmti
+ * @library /test/lib
+ * @modules java.instrument
+ * @compile TransformerDeadlockTest.java
+ * @run driver TransformerDeadlockTest
+ */
+
+import jdk.test.lib.process.ProcessTools;
+
+import java.lang.instrument.ClassFileTransformer;
+import java.lang.instrument.IllegalClassFormatException;
+import java.lang.instrument.Instrumentation;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.ProtectionDomain;
+
+
+public class TransformerDeadlockTest {
+
+    private static String manifest = "Premain-Class: " +
+                    TransformerDeadlockTest.Agent.class.getName() + "\n"
+                    + "Can-Retransform-Classes: true\n"
+                    + "Can-Retransform-Classes: true\n";
+
+    private static String CP = System.getProperty("test.classes");
+
+    public static void main(String args[]) throws Throwable {
+        String agentJar = buildAgent();
+        ProcessTools.executeProcess(
+                ProcessTools.createJavaProcessBuilder(
+                        "-javaagent:" + agentJar,
+                        TransformerDeadlockTest.Agent.class.getName())
+        ).shouldHaveExitValue(0);
+    }
+
+    private static String buildAgent() throws Exception {
+        Path jar = Files.createTempFile(Paths.get("."), null, ".jar");
+        String jarPath = jar.toAbsolutePath().toString();
+        ClassFileInstaller.writeJar(jarPath,
+                ClassFileInstaller.Manifest.fromString(manifest),
+                TransformerDeadlockTest.class.getName());
+        return jarPath;
+    }
+
+    public static class Agent implements ClassFileTransformer {
+        private static Instrumentation instrumentation;
+
+        public static void premain(String agentArgs, Instrumentation inst) {
+            instrumentation = inst;
+        }
+
+        @Override
+        public byte[] transform(
+                ClassLoader loader,
+                String className,
+                Class<?> classBeingRedefined,
+                ProtectionDomain protectionDomain,
+                byte[] classfileBuffer)
+                throws IllegalClassFormatException {
+
+            if (!TransformerDeadlockTest.class.getName().replace(".", "/").equals(className)) {
+                return null;
+            }
+            invokeRetransform();
+            return classfileBuffer;
+
+        }
+
+        public static void main(String[] args) throws Exception {
+            instrumentation.addTransformer(new TransformerDeadlockTest.Agent(), true);
+
+            try {
+                instrumentation.retransformClasses(TransformerDeadlockTest.class);
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            }
+        }
+
+        private static void invokeRetransform() {
+            try {
+                instrumentation.retransformClasses(TransformerDeadlockTest.class);
+            } catch (Exception e) {
+                throw new RuntimeException(e);
+            } finally {
+            }
+        }
+    }
+}