[vm, isolates] Avoid three-way deadlock during isolate exit, take 2.

Give up the tasks lock before interrupting to finalize marking. TEST=ci, tsan, rr chaos mode Bug: #59574 Change-Id: I9f4ca977354b3860897654790fceb8f0e2e25aab Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398580 Reviewed-by: Alexander Aprelev <[email protected]> Commit-Queue: Ryan Macnak <[email protected]>
dart-lang · Dec 3, 2024 · 130ae2a · 130ae2a
1 parent 85c339c
commit 130ae2a
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 17 deletions.
diff --git a/runtime/vm/heap/marker.cc b/runtime/vm/heap/marker.cc
@@ -1066,6 +1066,9 @@ class ConcurrentMarkTask : public ThreadPool::Task {
     // Exit isolate cleanly *before* notifying it, to avoid shutdown race.
     Thread::ExitIsolateGroupAsHelper(/*bypass_safepoint=*/true);
     // This marker task is done. Notify the original isolate.
+
+    Dart_Port interrupt_port = isolate_group_->interrupt_port();
+
     {
       MonitorLocker ml(page_space_->tasks_lock());
       page_space_->set_tasks(page_space_->tasks() - 1);
@@ -1076,10 +1079,12 @@ class ConcurrentMarkTask : public ThreadPool::Task {
       ASSERT(page_space_->phase() == PageSpace::kMarking);
       if (page_space_->concurrent_marker_tasks() == 0) {
         page_space_->set_phase(PageSpace::kAwaitingFinalization);
-        isolate_group_->ScheduleInterrupts(Thread::kVMInterrupt);
       }
       ml.NotifyAll();
     }
+
+    PortMap::PostMessage(Message::New(
+        interrupt_port, Smi::New(Thread::kVMInterrupt), Message::kOOBPriority));
   }
 
  private:

diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
@@ -442,6 +442,9 @@ IsolateGroup::~IsolateGroup() {
 void IsolateGroup::RegisterIsolate(Isolate* isolate) {
   SafepointWriteRwLocker ml(Thread::Current(), isolates_lock_.get());
   ASSERT(isolates_lock_->IsCurrentThreadWriter());
+  if (isolates_.IsEmpty()) {
+    interrupt_port_ = isolate->main_port();
+  }
   isolates_.Append(isolate);
   isolate_count_++;
 }
@@ -454,14 +457,14 @@ bool IsolateGroup::ContainsOnlyOneIsolate() {
   return isolate_count_ == 0 || isolate_count_ == 1;
 }
 
-void IsolateGroup::RunWithLockedGroup(std::function<void()> fun) {
-  SafepointWriteRwLocker ml(Thread::Current(), isolates_lock_.get());
-  fun();
-}
-
 void IsolateGroup::UnregisterIsolate(Isolate* isolate) {
   SafepointWriteRwLocker ml(Thread::Current(), isolates_lock_.get());
   isolates_.Remove(isolate);
+  if (isolates_.IsEmpty()) {
+    interrupt_port_ = ILLEGAL_PORT;
+  } else {
+    interrupt_port_ = isolates_.First()->main_port();
+  }
 }
 
 bool IsolateGroup::UnregisterIsolateDecrementCount() {
@@ -1424,6 +1427,13 @@ MessageHandler::MessageStatus IsolateMessageHandler::HandleMessage(
           }
         }
       }
+    } else if (msg.IsSmi()) {
+      uword interrupt_bits = Smi::Cast(msg).Value();
+      const Error& error =
+          Error::Handle(thread->HandleInterrupts(interrupt_bits));
+      if (!error.IsNull()) {
+        status = ProcessUnhandledException(error);
+      }
     }
   } else if (message->IsFinalizerInvocationRequest()) {
     const Object& msg_handler = Object::Handle(
@@ -1954,13 +1964,6 @@ void IsolateGroup::SetupImagePage(const uint8_t* image_buffer,
                          is_executable);
 }
 
-void IsolateGroup::ScheduleInterrupts(uword interrupt_bits) {
-  SafepointReadRwLocker ml(Thread::Current(), isolates_lock_.get());
-  for (Isolate* isolate : isolates_) {
-    isolate->ScheduleInterrupts(interrupt_bits);
-  }
-}
-
 void Isolate::ScheduleInterrupts(uword interrupt_bits) {
   // We take the threads lock here to ensure that the mutator thread does not
   // exit the isolate while we are trying to schedule interrupts on it.

diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h
@@ -327,9 +327,7 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
 
   bool ContainsOnlyOneIsolate();
 
-  void RunWithLockedGroup(std::function<void()> fun);
-
-  void ScheduleInterrupts(uword interrupt_bits);
+  Dart_Port interrupt_port() { return interrupt_port_; }
 
   ThreadRegistry* thread_registry() const { return thread_registry_.get(); }
   SafepointHandler* safepoint_handler() { return safepoint_handler_.get(); }
@@ -836,6 +834,7 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   std::unique_ptr<MutatorThreadPool> thread_pool_;
   std::unique_ptr<SafepointRwLock> isolates_lock_;
   IntrusiveDList<Isolate> isolates_;
+  RelaxedAtomic<Dart_Port> interrupt_port_ = ILLEGAL_PORT;
   intptr_t isolate_count_ = 0;
   bool initial_spawn_successful_ = false;
   Dart_LibraryTagHandler library_tag_handler_ = nullptr;

diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc
@@ -757,7 +757,10 @@ uword Thread::GetAndClearInterrupts() {
 }
 
 ErrorPtr Thread::HandleInterrupts() {
-  uword interrupt_bits = GetAndClearInterrupts();
+  return HandleInterrupts(GetAndClearInterrupts());
+}
+
+ErrorPtr Thread::HandleInterrupts(uword interrupt_bits) {
   if ((interrupt_bits & kVMInterrupt) != 0) {
     CheckForSafepoint();
     if (isolate_group()->store_buffer()->Overflowed()) {

diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h
@@ -507,6 +507,7 @@ class Thread : public ThreadState {
 
   void ScheduleInterrupts(uword interrupt_bits);
   ErrorPtr HandleInterrupts();
+  ErrorPtr HandleInterrupts(uword interrupt_bits);
   uword GetAndClearInterrupts();
   bool HasScheduledInterrupts() const {
     return (stack_limit_.load() & kInterruptsMask) != 0;