commit 35a07b2e73968d18a88a68e071b5ed8ec8837d56
author Ryan Macnak <rmacnak@google.com> Mon Dec 02 18:24:09 2024 +0000
committer Commit Queue <dart-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Dec 02 18:24:09 2024 +0000
tree 420523c1d13be58b580999247e3953599534ee56
parent 3feb138f6110741ec2aa0439276cf95ffe275a50

Revert "[vm, isolates] Avoid three-way deadlock during isolate exit."

This reverts commit e4e8360eba33c1a656f57e39c717f045165f124e.

Reason for revert: https://github.com/dart-lang/sdk/issues/59632

Original change's description:
> [vm, isolates] Avoid three-way deadlock during isolate exit.
>
> Running mutator
>   holds safepoint operation scope starting old space GC
>   waiting for old space tasks to reach 0
>
> Concurrent marker
>   holds old space tasks > 0
>   waiting for isolates_list_ lock to interrupt for finalization
>
> Exiting mutator
>   holds isolates_list_ lock_ to unregister isolate
>   waiting for safepoint to end TransitionVMToBlocked
>
> Reorder isolate [un]registeration to not need safepoint transition to acquire the isolates_list_ lock.
>
> TEST=ci
> Bug: https://github.com/dart-lang/sdk/issues/59574
> Change-Id: Ia98fabd654c880b253893a0598d2e26ed77f52da
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/397660
> Reviewed-by: Alexander Aprelev <aam@google.com>
> Commit-Queue: Ryan Macnak <rmacnak@google.com>

Bug: https://github.com/dart-lang/sdk/issues/59574
Change-Id: I529fc140160018c2b6768b108073b2e14ab7bdaa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398422
Reviewed-by: Alexander Aprelev <aam@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>

runtime/vm/isolate.cc

diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
index 887d560..0236145 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -454,6 +454,11 @@
   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);
@@ -1845,6 +1850,14 @@
 #undef ISOLATE_METRIC_INIT
 #endif  // !defined(PRODUCT)
 
+  // First we ensure we enter the isolate. This will ensure we're participating
+  // in any safepointing requests from this point on. Other threads requesting a
+  // safepoint operation will therefore wait until we've stopped.
+  //
+  // Though the [result] isolate is still in a state where no memory has been
+  // allocated, which means it's safe to GC the isolate group until here.
+  Thread::EnterIsolate(result);
+
   // Setup the isolate message handler.
   result->message_handler_ = new IsolateMessageHandler(result);
 
@@ -1872,14 +1885,6 @@
   // to vm-isolate objects, e.g. null)
   isolate_group->RegisterIsolate(result);
 
-  // Now we enter the isolate. This will ensure we're participating in any
-  // safepointing requests from this point on. Other threads requesting a
-  // safepoint operation will therefore wait until we've stopped.
-  //
-  // Though the [result] isolate is still in a state where no memory has been
-  // allocated, which means it's safe to GC the isolate group until here.
-  Thread::EnterIsolate(result);
-
   if (api_flags.is_service_isolate) {
     ASSERT(!ServiceIsolate::Exists());
     ServiceIsolate::SetServiceIsolate(result);
@@ -2614,15 +2619,14 @@
   Dart_IsolateCleanupCallback cleanup = isolate->on_cleanup_callback();
   auto callback_data = isolate->init_callback_data_;
 
+  // From this point on the isolate is no longer visited by GC (which is ok,
+  // since we're just going to delete it anyway).
+  isolate_group->UnregisterIsolate(isolate);
+
   // From this point on the isolate doesn't participate in safepointing
   // requests anymore.
   ASSERT(!Thread::Current()->HasActiveState());
   Thread::ExitIsolate(/*isolate_shutdown=*/true);
-  ASSERT(Thread::Current() == nullptr);
-
-  // From this point on the isolate is no longer visited by GC (which is ok,
-  // since we're just going to delete it anyway).
-  isolate_group->UnregisterIsolate(isolate);
 
   // Now it's safe to delete the isolate.
   delete isolate;

runtime/vm/isolate.h

diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h
index 8b7e148..a1d43d6 100644
--- a/runtime/vm/isolate.h
+++ b/runtime/vm/isolate.h
@@ -327,6 +327,8 @@
 
   bool ContainsOnlyOneIsolate();
 
+  void RunWithLockedGroup(std::function<void()> fun);
+
   void ScheduleInterrupts(uword interrupt_bits);
 
   ThreadRegistry* thread_registry() const { return thread_registry_.get(); }

runtime/vm/port.cc

diff --git a/runtime/vm/port.cc b/runtime/vm/port.cc
index 547959d..a6d0375 100644
--- a/runtime/vm/port.cc
+++ b/runtime/vm/port.cc
@@ -59,6 +59,10 @@
     return ILLEGAL_PORT;
   }
 
+#if defined(DEBUG)
+  handler->CheckAccess();
+#endif
+
   const Dart_Port port = AllocatePort();
   if (auto ports = handler->ports(ml)) {
     ports->Insert(PortHandler::PortSetEntry{port});