[vm/concurrency] Remove redundant isolates list, ensure shutdown procedure waits until the isolates actually got deleted

Issue https://github.com/dart-lang/sdk/issues/36097

Change-Id: If24affbb838eff8d80e5d448eac7455b3ffcb3a1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135062
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>

runtime/vm/dart.cc

@@ -246,12 +246,12 @@ char* Dart::Init(const uint8_t* vm_isolate_snapshot,
     // really an isolate itself - it acts more as a container for VM-global
     // objects.
     std::unique_ptr<IsolateGroupSource> source(
-        new IsolateGroupSource(nullptr, "vm-isolate", vm_isolate_snapshot,
+        new IsolateGroupSource(nullptr, kVmIsolateName, vm_isolate_snapshot,
                                instructions_snapshot, nullptr, -1, api_flags));
     auto group = new IsolateGroup(std::move(source), /*embedder_data=*/nullptr);
     IsolateGroup::RegisterIsolateGroup(group);
     vm_isolate_ =
-        Isolate::InitIsolate("vm-isolate", group, api_flags, is_vm_isolate);
+        Isolate::InitIsolate(kVmIsolateName, group, api_flags, is_vm_isolate);
     group->set_initial_spawn_successful();
 
     // Verify assumptions about executing in the VM isolate.
@@ -405,31 +405,43 @@ char* Dart::Init(const uint8_t* vm_isolate_snapshot,
   return NULL;
 }
 
-bool Dart::HasApplicationIsolateLocked() {
+static void DumpAliveIsolates(intptr_t num_attempts,
-  for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL;
+                              bool only_aplication_isolates) {
-       isolate = isolate->next_) {
+  IsolateGroup::ForEach([&](IsolateGroup* group) {
-    if (!Isolate::IsVMInternalIsolate(isolate)) return true;
+    group->ForEachIsolate([&](Isolate* isolate) {
-  }
+      if (!only_aplication_isolates || !Isolate::IsVMInternalIsolate(isolate)) {
-  return false;
+        OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n",
+                     num_attempts, isolate->name());
+      }
+    });
+  });
+}
+
+static bool OnlyVmIsolateLeft() {
+  intptr_t count = 0;
+  bool found_vm_isolate = false;
+  IsolateGroup::ForEach([&](IsolateGroup* group) {
+    group->ForEachIsolate([&](Isolate* isolate) {
+      count++;
+      if (isolate == Dart::vm_isolate()) {
+        found_vm_isolate = true;
+      }
+    });
+  });
+  return count == 1 && found_vm_isolate;
 }
 
 // This waits until only the VM, service and kernel isolates are in the list.
 void Dart::WaitForApplicationIsolateShutdown() {
   ASSERT(!Isolate::creation_enabled_);
-  MonitorLocker ml(Isolate::isolates_list_monitor_);
+  MonitorLocker ml(Isolate::isolate_creation_monitor_);
   intptr_t num_attempts = 0;
-  while (HasApplicationIsolateLocked()) {
+  while (Isolate::application_isolates_count_ > 1) {
     Monitor::WaitResult retval = ml.Wait(1000);
     if (retval == Monitor::kTimedOut) {
       num_attempts += 1;
       if (num_attempts > 10) {
-        for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL;
+        DumpAliveIsolates(num_attempts, /*only_application_isolates=*/true);
-             isolate = isolate->next_) {
-          if (!Isolate::IsVMInternalIsolate(isolate)) {
-            OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n",
-                         num_attempts, isolate->name_);
-          }
-        }
       }
     }
   }
@@ -438,22 +450,19 @@ void Dart::WaitForApplicationIsolateShutdown() {
 // This waits until only the VM isolate remains in the list.
 void Dart::WaitForIsolateShutdown() {
   ASSERT(!Isolate::creation_enabled_);
-  MonitorLocker ml(Isolate::isolates_list_monitor_);
+  MonitorLocker ml(Isolate::isolate_creation_monitor_);
   intptr_t num_attempts = 0;
-  while ((Isolate::isolates_list_head_ != NULL) &&
+  while (Isolate::total_isolates_count_ > 1) {
-         (Isolate::isolates_list_head_->next_ != NULL)) {
     Monitor::WaitResult retval = ml.Wait(1000);
     if (retval == Monitor::kTimedOut) {
       num_attempts += 1;
       if (num_attempts > 10) {
-        for (Isolate* isolate = Isolate::isolates_list_head_; isolate != NULL;
+        DumpAliveIsolates(num_attempts, /*only_application_isolates=*/false);
-             isolate = isolate->next_)
-          OS::PrintErr("Attempt:%" Pd " waiting for isolate %s to check in\n",
-                       num_attempts, isolate->name_);
       }
     }
   }
-  ASSERT(Isolate::isolates_list_head_ == Dart::vm_isolate());
+
+  ASSERT(OnlyVmIsolateLeft());
 }
 
 char* Dart::Cleanup() {
@@ -940,11 +949,22 @@ void Dart::ShutdownIsolate(Isolate* isolate) {
 
 void Dart::ShutdownIsolate() {
   Isolate* isolate = Isolate::Current();
+  const bool is_application_isolate = !Isolate::IsVMInternalIsolate(isolate);
   isolate->Shutdown();
   if (KernelIsolate::IsKernelIsolate(isolate)) {
     KernelIsolate::SetKernelIsolate(NULL);
   }
   delete isolate;
+
+  // Only now do we know for sure that the isolate and all it's resources have
+  // been deleted. So we can let any potential Dart::Cleanup() know it's safe to
+  // proceed shutdown of the VM.
+  Isolate::MarkIsolateDead(is_application_isolate);
+}
+
+bool Dart::VmIsolateNameEquals(const char* name) {
+  ASSERT(name != NULL);
+  return (strcmp(name, kVmIsolateName) == 0);
 }
 
 int64_t Dart::UptimeMicros() {

runtime/vm/dart.h

@@ -66,6 +66,7 @@ class Dart : public AllStatic {
 
   static Isolate* vm_isolate() { return vm_isolate_; }
   static ThreadPool* thread_pool() { return thread_pool_; }
+  static bool VmIsolateNameEquals(const char* name);
 
   static int64_t UptimeMicros();
   static int64_t UptimeMillis() {
@@ -129,9 +130,10 @@ class Dart : public AllStatic {
   static bool non_nullable_flag() { return true; }
 
  private:
+  static constexpr const char* kVmIsolateName = "vm-isolate";
+
   static void WaitForIsolateShutdown();
   static void WaitForApplicationIsolateShutdown();
-  static bool HasApplicationIsolateLocked();
 
   static Isolate* vm_isolate_;
   static int64_t start_time_micros_;

runtime/vm/isolate.cc

@@ -1380,10 +1380,10 @@ void Isolate::InitVM() {
   shutdown_callback_ = nullptr;
   cleanup_callback_ = nullptr;
   cleanup_group_callback_ = nullptr;
-  if (isolates_list_monitor_ == nullptr) {
+  if (isolate_creation_monitor_ == nullptr) {
-    isolates_list_monitor_ = new Monitor();
+    isolate_creation_monitor_ = new Monitor();
   }
-  ASSERT(isolates_list_monitor_ != nullptr);
+  ASSERT(isolate_creation_monitor_ != nullptr);
   EnableIsolateCreation();
 }
 
@@ -1392,6 +1392,7 @@ Isolate* Isolate::InitIsolate(const char* name_prefix,
                               const Dart_IsolateFlags& api_flags,
                               bool is_vm_isolate) {
   Isolate* result = new Isolate(isolate_group, api_flags);
+  result->BuildName(name_prefix);
   ASSERT(result != nullptr);
 
 #if !defined(PRODUCT)
@@ -1458,7 +1459,6 @@ Isolate* Isolate::InitIsolate(const char* name_prefix,
   result->set_pause_capability(result->random()->NextUInt64());
   result->set_terminate_capability(result->random()->NextUInt64());
 
-  result->BuildName(name_prefix);
 #if !defined(PRODUCT)
   result->debugger_ = new Debugger(result);
 #endif
@@ -1476,7 +1476,7 @@ Isolate* Isolate::InitIsolate(const char* name_prefix,
 #endif  // !PRODUCT
 
   // Add to isolate list. Shutdown and delete the isolate on failure.
-  if (!AddIsolateToList(result)) {
+  if (!TryMarkIsolateReady(result)) {
     result->LowLevelShutdown();
     Thread::ExitIsolate();
     if (KernelIsolate::IsKernelIsolate(result)) {
@@ -2224,12 +2224,7 @@ void Isolate::Shutdown() {
   // Don't allow anymore dart code to execution on this isolate.
   thread->ClearStackLimit();
 
-  // Remove this isolate from the list *before* we start tearing it down, to
-  // avoid exposing it in a state of decay.
-  RemoveIsolateFromList(this);
-
   {
-    // After removal from isolate list. Before tearing down the heap.
     StackZone zone(thread);
     HandleScope handle_scope(thread);
     ServiceIsolate::SendIsolateShutdownMessage();
@@ -2265,6 +2260,7 @@ void Isolate::Shutdown() {
 #endif  // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)
 
   // Then, proceed with low-level teardown.
+  Isolate::UnMarkIsolateReady(this);
   LowLevelShutdown();
 
 #if defined(DEBUG)
@@ -2295,8 +2291,9 @@ Dart_IsolateShutdownCallback Isolate::shutdown_callback_ = nullptr;
 Dart_IsolateCleanupCallback Isolate::cleanup_callback_ = nullptr;
 Dart_IsolateGroupCleanupCallback Isolate::cleanup_group_callback_ = nullptr;
 
-Monitor* Isolate::isolates_list_monitor_ = nullptr;
+Monitor* Isolate::isolate_creation_monitor_ = nullptr;
-Isolate* Isolate::isolates_list_head_ = nullptr;
+intptr_t Isolate::application_isolates_count_ = 0;
+intptr_t Isolate::total_isolates_count_ = 0;
 bool Isolate::creation_enabled_ = false;
 
 RwLock* IsolateGroup::isolate_groups_rwlock_ = nullptr;
@@ -3026,113 +3023,99 @@ void Isolate::VisitIsolates(IsolateVisitor* visitor) {
   if (visitor == nullptr) {
     return;
   }
-  // The visitor could potentially run code that could safepoint so use
+  IsolateGroup::ForEach([&](IsolateGroup* group) {
-  // SafepointMonitorLocker to ensure the lock has safepoint checks.
+    group->ForEachIsolate(
-  SafepointMonitorLocker ml(isolates_list_monitor_);
+        [&](Isolate* isolate) { visitor->VisitIsolate(isolate); });
-  Isolate* current = isolates_list_head_;
+  });
-  while (current != nullptr) {
-    visitor->VisitIsolate(current);
-    current = current->next_;
-  }
 }
 
 intptr_t Isolate::IsolateListLength() {
-  MonitorLocker ml(isolates_list_monitor_);
+  MonitorLocker ml(isolate_creation_monitor_);
-  intptr_t count = 0;
+  return total_isolates_count_;
-  Isolate* current = isolates_list_head_;
-  while (current != nullptr) {
-    count++;
-    current = current->next_;
-  }
-  return count;
 }
 
 Isolate* Isolate::LookupIsolateByPort(Dart_Port port) {
-  MonitorLocker ml(isolates_list_monitor_);
+  Isolate* match = nullptr;
-  Isolate* current = isolates_list_head_;
+  IsolateGroup::ForEach([&](IsolateGroup* group) {
-  while (current != nullptr) {
+    group->ForEachIsolate([&](Isolate* isolate) {
-    if (current->main_port() == port) {
+      if (isolate->main_port() == port) {
-      return current;
+        match = isolate;
-    }
+      }
-    current = current->next_;
+    });
-  }
+  });
-  return nullptr;
+  return match;
 }
 
 std::unique_ptr<char[]> Isolate::LookupIsolateNameByPort(Dart_Port port) {
-  MonitorLocker ml(isolates_list_monitor_);
+  MonitorLocker ml(isolate_creation_monitor_);
-  Isolate* current = isolates_list_head_;
+  std::unique_ptr<char[]> result;
-  while (current != nullptr) {
+  IsolateGroup::ForEach([&](IsolateGroup* group) {
-    if (current->main_port() == port) {
+    group->ForEachIsolate([&](Isolate* isolate) {
-      const size_t len = strlen(current->name()) + 1;
+      if (isolate->main_port() == port) {
-      auto result = std::unique_ptr<char[]>(new char[len]);
+        const size_t len = strlen(isolate->name()) + 1;
-      strncpy(result.get(), current->name(), len);
+        result = std::unique_ptr<char[]>(new char[len]);
-      return result;
+        strncpy(result.get(), isolate->name(), len);
-    }
+      }
-    current = current->next_;
+    });
-  }
+  });
-  return std::unique_ptr<char[]>();
+  return result;
 }
 
-bool Isolate::AddIsolateToList(Isolate* isolate) {
+bool Isolate::TryMarkIsolateReady(Isolate* isolate) {
-  MonitorLocker ml(isolates_list_monitor_);
+  MonitorLocker ml(isolate_creation_monitor_);
   if (!creation_enabled_) {
     return false;
   }
-  ASSERT(isolate != nullptr);
+  total_isolates_count_++;
-  ASSERT(isolate->next_ == nullptr);
+  if (!Isolate::IsVMInternalIsolate(isolate)) {
-  isolate->next_ = isolates_list_head_;
+    application_isolates_count_++;
-  isolates_list_head_ = isolate;
+  }
+  isolate->accepts_messages_ = true;
   return true;
 }
 
-void Isolate::RemoveIsolateFromList(Isolate* isolate) {
+void Isolate::UnMarkIsolateReady(Isolate* isolate) {
-  MonitorLocker ml(isolates_list_monitor_);
+  MonitorLocker ml(isolate_creation_monitor_);
-  ASSERT(isolate != nullptr);
+  ASSERT(total_isolates_count_ > 0);
-  if (isolate == isolates_list_head_) {
+  isolate->accepts_messages_ = false;
-    isolates_list_head_ = isolate->next_;
+}
-    if (!creation_enabled_) {
+
-      ml.Notify();
+void Isolate::MarkIsolateDead(bool is_application_isolate) {
-    }
+  MonitorLocker ml(isolate_creation_monitor_);
-    return;
+  ASSERT(total_isolates_count_ > 0);
+  total_isolates_count_--;
+  if (is_application_isolate) {
+    ASSERT(application_isolates_count_ > 0);
+    application_isolates_count_--;
   }
-  Isolate* previous = nullptr;
+  if (!creation_enabled_) {
-  Isolate* current = isolates_list_head_;
+    ml.Notify();
-  while (current != nullptr) {
-    if (current == isolate) {
-      ASSERT(previous != nullptr);
-      previous->next_ = current->next_;
-      if (!creation_enabled_) {
-        ml.Notify();
-      }
-      return;
-    }
-    previous = current;
-    current = current->next_;
   }
-  // If we are shutting down the VM, the isolate may not be in the list.
-  ASSERT(!creation_enabled_);
 }
 
 void Isolate::DisableIsolateCreation() {
-  MonitorLocker ml(isolates_list_monitor_);
+  MonitorLocker ml(isolate_creation_monitor_);
   creation_enabled_ = false;
 }
 
 void Isolate::EnableIsolateCreation() {
-  MonitorLocker ml(isolates_list_monitor_);
+  MonitorLocker ml(isolate_creation_monitor_);
   creation_enabled_ = true;
 }
 
 bool Isolate::IsolateCreationEnabled() {
-  MonitorLocker ml(isolates_list_monitor_);
+  MonitorLocker ml(isolate_creation_monitor_);
   return creation_enabled_;
 }
 
 bool Isolate::IsVMInternalIsolate(const Isolate* isolate) {
-  return (isolate == Dart::vm_isolate()) ||
+  // We use a name comparison here because this method can be called during
-         ServiceIsolate::IsServiceIsolateDescendant(isolate) ||
+  // shutdown, where the actual isolate pointers might've already been cleared.
-         KernelIsolate::IsKernelIsolate(isolate);
+  return Dart::VmIsolateNameEquals(isolate->name()) ||
+#if !defined(DART_PRECOMPILED_RUNTIME)
+         KernelIsolate::NameEquals(isolate->name()) ||
+#endif
+         ServiceIsolate::NameEquals(isolate->name());
 }
 
 void Isolate::KillLocked(LibMsgId msg_id) {
@@ -3189,7 +3172,9 @@ class IsolateKillerVisitor : public IsolateVisitor {
   void VisitIsolate(Isolate* isolate) {
     ASSERT(isolate != nullptr);
     if (ShouldKill(isolate)) {
-      isolate->KillLocked(msg_id_);
+      if (isolate->AcceptsMessagesLocked()) {
+        isolate->KillLocked(msg_id_);
+      }
     }
   }
 
@@ -3206,11 +3191,15 @@ class IsolateKillerVisitor : public IsolateVisitor {
 };
 
 void Isolate::KillAllIsolates(LibMsgId msg_id) {
+  MonitorLocker ml(isolate_creation_monitor_);
+
   IsolateKillerVisitor visitor(msg_id);
   VisitIsolates(&visitor);
 }
 
 void Isolate::KillIfExists(Isolate* isolate, LibMsgId msg_id) {
+  MonitorLocker ml(isolate_creation_monitor_);
+
   IsolateKillerVisitor visitor(isolate, msg_id);
   VisitIsolates(&visitor);
 }

runtime/vm/isolate.h

@@ -1087,7 +1087,7 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
                               const Dart_IsolateFlags& api_flags,
                               bool is_vm_isolate = false);
 
-  // The isolates_list_monitor_ should be held when calling Kill().
+  // The isolate_creation_monitor_ should be held when calling Kill().
   void KillLocked(LibMsgId msg_id);
 
   void LowLevelShutdown();
@@ -1277,9 +1277,6 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
 
   RawError* sticky_error_;
 
-  // Isolate list next pointer.
-  Isolate* next_ = nullptr;
-
   // Protect access to boxed_field_list_.
   Mutex field_list_mutex_;
   // List of fields that became boxed and that trigger deoptimization.
@@ -1303,6 +1300,11 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
   std::unique_ptr<WeakTable> forward_table_new_;
   std::unique_ptr<WeakTable> forward_table_old_;
 
+  // Signals whether the isolate can receive messages (e.g. KillAllIsolates can
+  // send a kill message).
+  // This is protected by [isolate_creation_monitor_].
+  bool accepts_messages_ = false;
+
   static Dart_IsolateGroupCreateCallback create_group_callback_;
   static Dart_InitializeIsolateCallback initialize_callback_;
   static Dart_IsolateShutdownCallback shutdown_callback_;
@@ -1314,12 +1316,19 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
 #endif
 
   // Manage list of existing isolates.
-  static bool AddIsolateToList(Isolate* isolate);
+  static bool TryMarkIsolateReady(Isolate* isolate);
-  static void RemoveIsolateFromList(Isolate* isolate);
+  static void UnMarkIsolateReady(Isolate* isolate);
+  static void MarkIsolateDead(bool is_application_isolate);
+  bool AcceptsMessagesLocked() {
+    ASSERT(isolate_creation_monitor_->IsOwnedByCurrentThread());
+    return accepts_messages_;
+  }
 
-  // This monitor protects isolates_list_head_, and creation_enabled_.
+  // This monitor protects application_isolates_count_, total_isolates_count_,
-  static Monitor* isolates_list_monitor_;
+  // creation_enabled_.
-  static Isolate* isolates_list_head_;
+  static Monitor* isolate_creation_monitor_;
+  static intptr_t application_isolates_count_;
+  static intptr_t total_isolates_count_;
   static bool creation_enabled_;
 
 #define REUSABLE_FRIEND_DECLARATION(name)                                      \

runtime/vm/service_isolate.cc

@@ -363,7 +363,6 @@ class RunServiceTask : public ThreadPool::Task {
 
       free(error);
       error = nullptr;
-      ServiceIsolate::SetServiceIsolate(NULL);
       ServiceIsolate::InitializingFailed(formatted_error);
       return;
     }
@@ -419,9 +418,6 @@ class RunServiceTask : public ThreadPool::Task {
       }
       Dart::RunShutdownCallback();
     }
-    ASSERT(ServiceIsolate::IsServiceIsolate(I));
-    ServiceIsolate::SetServiceIsolate(NULL);
-    ServiceIsolate::SetServicePort(ILLEGAL_PORT);
 
     // Shut the isolate down.
     Dart::ShutdownIsolate(I);