Block on concurrent redefinitions instead of returning error

We don't support concurrent redefinitions. We return an error if there
is already a redefinition in progress. This isn't strictly according to
the spec. This CL changes it to block on concurrent redefinition
requests.

Bug: 376717110
Test: art/test.py
Change-Id: I0f981392f3f66e2310e7efd12ef82dfba7c49a23

1 files changed, 24 insertions, 23 deletions

diff --git a/openjdkjvmti/ti_redefine.cc b/openjdkjvmti/ti_redefine.cc
index 19e9adff57..8e11d592e5 100644
--- a/openjdkjvmti/ti_redefine.cc
+++ b/openjdkjvmti/ti_redefine.cc
@@ -343,23 +343,7 @@ namespace {
 // We need to make sure we only have one redefinition in progress. Redefining involves
 // re-verification and potentially new allocations among other things. So we only allow one
 // redefinition at a time.
-static art::Mutex redefinition_lock("JVMTI Redefinition lock", art::LockLevel::kGenericBottomLock);
-static bool redefinition_in_progress GUARDED_BY(redefinition_lock) = false;
-
-bool canHandleRedefinition(art::Thread* self) {
-  art::MutexLock mu(self, redefinition_lock);
-  if (redefinition_in_progress) {
-    return false;
-  }
-  redefinition_in_progress = true;
-  return true;
-}
-
-void finishRedefinition(art::Thread* self) {
-  art::MutexLock mu(self, redefinition_lock);
-  DCHECK_EQ(redefinition_in_progress, true);
-  redefinition_in_progress = false;
-}
+static std::mutex redefinition_lock;
 }  // namespace
 
 template <RedefinitionType kType>
@@ -660,15 +644,34 @@ jvmtiError Redefiner::RedefineClassesDirect(ArtJvmTiEnv* env,
     // We don't actually need to do anything. Just return OK.
     return OK;
   }
+
+  // Take a lock to avoid any concurrent redefinitions.
+  // TODO(mythria): It is hard to reason that it is safe to hold locks here. It is probably okay,
+  // since the thread is suspended and we know the thread isn't in the middle of allocations. The
+  // current implementation of redefinition is prone to deadlocks. For example, we pause allocations
+  // and then allocate new objects which could trigger a GC. This is unsafe. See b/359829378 for
+  // more details. Ideally we should rework the code so that:
+  // 1. Estimate the size required for the new allocations
+  // 2. Ensure we have the required space
+  // 3. Acquire any locks required (this would also include the lock to prevent
+  // concurrent redefinitions)
+  // 3. SuspendAll the threads
+  // 4. If the estimated size is no longer sufficient - retry from 1.
+  // 5. Finish redefinition.
+  //
+  // Step 4 is required because there might be allocations after we have estimated and before we
+  // suspend all threads. This isn't expected to be frequent so we shouldn't usually need to retry
+  // multiple times.
+  // Using a lock here is a short-term fix to block on concurrent redefinitions (instead of
+  // returning an error) while we rework the redefinition code.
+  // art::MutexLock lg(self, redefinition_lock);
+  std::lock_guard<std::mutex> lg(redefinition_lock);
+
   // We need to fiddle with the verification class flags. To do this we need to make sure there are
   // no concurrent redefinitions of the same class at the same time. For simplicity and because
   // this is not expected to be a common occurrence we will just wrap the whole thing in a TOP-level
   // lock.
   Redefiner r(env, runtime, self, type, error_msg);
-  if (!canHandleRedefinition(self)) {
-    r.RecordFailure(ERR(INTERNAL), "Another redefinition is in progress");
-    return r.result_;
-  }
 
   // Stop JIT for the duration of this redefine since the JIT might concurrently compile a method we
   // are going to redefine.
@@ -681,13 +684,11 @@ jvmtiError Redefiner::RedefineClassesDirect(ArtJvmTiEnv* env,
     if (def.IsModified()) {
       jvmtiError res = r.AddRedefinition(env, def);
       if (res != OK) {
-        finishRedefinition(self);
         return res;
       }
     }
   }
   jvmtiError res = r.Run();
-  finishRedefinition(self);
   return res;
 }