[vm/isolate_api] Ensure isolate acquisition fails gracefully when target isolate is exiting.

Introduce `is_acquirable` isolate property that tracks whether isolate can be entered, avoid a `pool_` check that does not work correctly as isolate being shutdown.

BUG=https://github.com/dart-lang/sdk/issues/63515
BUG=https://github.com/dart-lang/sdk/issues/63514
TEST=ci

Change-Id: I84577ad082d87ead8cdb9f591fabdf918e2f4bd0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/509221
Reviewed-by: Slava Egorov <vegorov@google.com>

runtime/lib/isolate.cc

@@ -577,6 +577,8 @@ class IsolateAcquireScope : public ValueObject {
         return "Isolate is pinned to a different thread already";
       case IsolateAcquireResult::BUSY:
         return "Isolate is busy, running on a different thread";
+      case IsolateAcquireResult::HAS_MESSAGE_LOOP_OR_UNAVAILABLE:
+        return "Isolate has a message loop running or otherwise unavailable.";
       default:
         UNREACHABLE();
     }
@@ -638,13 +640,6 @@ DEFINE_NATIVE_ENTRY(Isolate_runEventLoopSync_, 0, 1) {
 
   Dart_Port control_port_id = isolate_control_port.Id();
 
-  if (PortMap::HasEventLoopRunning(control_port_id)) {
-    const auto& error =
-        String::Handle(String::New("Isolate has a message loop running."));
-    Exceptions::ThrowStateError(error);
-    UNREACHABLE();
-  }
-
   Error& result_error = Error::Handle();
   Thread::ExitIsolateGroupAsMutator(/*bypass_safepoint=*/false);
   {
@@ -714,13 +709,6 @@ DEFINE_NATIVE_ENTRY(Isolate_runSync_, 1, 2) {
       UNREACHABLE();
     }
   } else {
-    if (PortMap::HasEventLoopRunning(control_port_id)) {
-      const auto& error =
-          String::Handle(String::New("Isolate has a message loop running."));
-      Exceptions::ThrowStateError(error);
-      UNREACHABLE();
-    }
-
     if (isolate != nullptr) {
       ASSERT(Thread::Current()->isolate() == isolate);
       Thread::ExitIsolate(/*isolate_shutdown=*/false);

runtime/vm/isolate.cc

@@ -1167,6 +1167,11 @@ class IsolateMessageHandler : public MessageHandler {
   ErrorPtr HandleLibMessage(const Array& message);
 
   MessageStatus ProcessUnhandledException(const Error& result);
+
+  void set_is_scheduled() override {
+    ASSERT(isolate_ != nullptr);
+    isolate_->set_is_not_acquirable();
+  }
   Isolate* isolate_;
 };
 

runtime/vm/isolate.h

@@ -1112,6 +1112,7 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
 
   void set_message_notify_callback(Dart_MessageNotifyCallback value) {
     message_notify_callback_.store(value, std::memory_order_release);
+    is_acquirable_ = false;
   }
 
   void set_on_shutdown_callback(Dart_IsolateShutdownCallback value) {
@@ -1301,6 +1302,8 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
   void clear_is_permanently_pinned_for_testing_only() {
     is_permanently_pinned_ = false;
   }
+  bool is_acquirable() { return is_acquirable_; }
+  void set_is_not_acquirable() { is_acquirable_ = false; }
 
   static void SetCreateGroupCallback(Dart_IsolateGroupCreateCallback cb) {
     create_group_callback_ = cb;
@@ -1709,6 +1712,7 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
   intptr_t ffi_callback_keep_alive_counter_ = 0;
   RelaxedAtomic<ThreadId> owner_thread_ = OSThread::kInvalidThreadId;
   bool is_permanently_pinned_ = false;
+  bool is_acquirable_ = true;
 
   ErrorPtr sticky_error_;
 

runtime/vm/message_handler.cc

@@ -100,6 +100,7 @@ bool MessageHandler::Run(ThreadPool* pool,
   }
   ASSERT(pool_ == nullptr);
   pool_ = pool;
+  set_is_scheduled();
   end_callback_ = end_callback;
   callback_data_ = data;
   task_running_ = true;

runtime/vm/message_handler.h

@@ -146,7 +146,7 @@ class MessageHandler : public PortHandler {
   void PostMessage(std::unique_ptr<Message> message,
                    bool before_events = false) override;
 
-  bool is_scheduled() { return pool_ != nullptr; }
+  virtual void set_is_scheduled() {}
 
  private:
   template <typename GCVisitorType>

runtime/vm/port.cc

@@ -244,6 +244,10 @@ IsolateAcquireResult PortMap::AcquireIsolateByControlPort(Dart_Port target_port,
   ASSERT(target_handler != nullptr);
   auto target_isolate = target_handler->isolate();
 
+  if (!target_handler->isolate()->is_acquirable()) {
+    return IsolateAcquireResult::HAS_MESSAGE_LOOP_OR_UNAVAILABLE;
+  }
+
   if (!target_isolate->TryAcquireOwnership()) {
     return target_isolate->is_permanently_pinned()
                ? IsolateAcquireResult::PINNED_TO_ANOTHER_THREAD
@@ -254,25 +258,6 @@ IsolateAcquireResult PortMap::AcquireIsolateByControlPort(Dart_Port target_port,
   return IsolateAcquireResult::SUCCESS;
 }
 
-bool PortMap::HasEventLoopRunning(Dart_Port id) {
-  Locker ml;  // isolates are not exiting while we hold this lock
-  if (ports_ == nullptr) {
-    return false;
-  }
-  auto it = ports_->TryLookup(id);
-  if (it == ports_->end()) {
-    return false;
-  }
-  auto target_handler = (*it).handler;
-  ASSERT(target_handler != nullptr);
-  auto isolate = target_handler->isolate();
-  if (isolate->message_notify_callback() != nullptr) {
-    return true;
-  }
-  auto message_handler = isolate->message_handler();
-  return message_handler != nullptr && message_handler->is_scheduled();
-}
-
 #if defined(TESTING)
 bool PortMap::HasPorts(MessageHandler* handler) {
   Locker ml;

runtime/vm/port.h

@@ -28,6 +28,7 @@ enum class IsolateAcquireResult {
   ISOLATE_NOT_AVAILABLE,
   BUSY,
   PINNED_TO_ANOTHER_THREAD,
+  HAS_MESSAGE_LOOP_OR_UNAVAILABLE,
 };
 
 class PortMap : public AllStatic {
@@ -61,8 +62,6 @@ class PortMap : public AllStatic {
   // Returns true if the port is owned by somebody.
   static bool IsOwned(Dart_Port id);
 
-  static bool HasEventLoopRunning(Dart_Port id);
-
   static IsolateAcquireResult AcquireIsolateByControlPort(Dart_Port target_port,
                                                           Isolate** p_isolate);