[S] Change in dart/sdk[main]: [vm/isolate_api] Ensure isolate acquisition fails gracefully when tar...
1 view
Skip to first unread message
Slava Egorov (Gerrit)
Jun 9, 2026, 8:06:50 AM (2 days ago) Jun 9
to Alexander Aprelev, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org, vm-...@dartlang.org
Attention needed from Alexander Aprelev
Slava Egorov added 1 comment![]( "Open in Gerrit")
File runtime/vm/port.cc
Line 257, Patchset 5 (Latest):
bool PortMap::HasEventLoopRunning(Dart_Port id) {Slava Egorov . unresolved
Maybe we can just add a field to isolate which says: `is_aquirable_` (or something like that) which is set to `true` originally but then set to `false` is we ever schedule a message handler for the isolate or set the message_notify_callback - and it never goes back to `true`.
Then you should just check that flag when trying to acquire ownership - and we don't need this code at all.
Related details
Attention is currently required from:
- Alexander Aprelev
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Jun 9, 2026, 1:24:43 PM (2 days ago) Jun 9
to Alexander Aprelev, Slava Egorov, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org, vm-...@dartlang.org
Attention needed from Slava Egorov
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
File runtime/vm/port.cc
Maybe we can just add a field to isolate which says: `is_aquirable_` (or something like that) which is set to `true` originally but then set to `false` is we ever schedule a message handler for the isolate or set the message_notify_callback - and it never goes back to `true`.
Then you should just check that flag when trying to acquire ownership - and we don't need this code at all.
Attention is currently required from:
- Slava Egorov
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Jun 10, 2026, 6:48:15 AM (yesterday) Jun 10
to Alexander Aprelev, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org, vm-...@dartlang.org
Attention needed from Alexander Aprelev
Slava Egorov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
2 comments
File runtime/vm/message_handler.h
Line 149, Patchset 6 (Latest):
bool is_scheduled() { return pool_ != nullptr; }Slava Egorov . unresolved
This is probably not needed anymore.
File runtime/vm/port.cc
Line 257, Patchset 6 (Latest):
bool PortMap::IsAcquirable(Dart_Port id) {Slava Egorov . unresolved
Why not merge it into the `AcquireIsolateByControlPort`?
Related details
Attention is currently required from:
- Alexander Aprelev
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Jun 10, 2026, 11:32:43 AM (yesterday) Jun 10
to Alexander Aprelev, Slava Egorov, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org, vm-...@dartlang.org
Alexander Aprelev voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
3 comments
Patchset-level comments
File runtime/vm/message_handler.h
This is probably not needed anymore.
Alexander Aprelev
Done
File runtime/vm/port.cc
Why not merge it into the `AcquireIsolateByControlPort`?
Alexander Aprelev
Ah, good idea, done.
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Jun 10, 2026, 12:59:11 PM (yesterday) Jun 10
to Alexander Aprelev, Slava Egorov, dart-...@luci-project-accounts.iam.gserviceaccount.com, rev...@dartlang.org, vm-...@dartlang.org
Alexander Aprelev submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: runtime/vm/port.h
Insertions: 1, Deletions: 2.
@@ -28,6 +28,7 @@
ISOLATE_NOT_AVAILABLE,
BUSY,
PINNED_TO_ANOTHER_THREAD,
+ HAS_MESSAGE_LOOP_OR_UNAVAILABLE,
};
class PortMap : public AllStatic {
@@ -61,8 +62,6 @@
// Returns true if the port is owned by somebody.
static bool IsOwned(Dart_Port id);
- static bool IsAcquirable(Dart_Port id);
-
static IsolateAcquireResult AcquireIsolateByControlPort(Dart_Port target_port,
Isolate** p_isolate);
```
```
The name of the file: runtime/lib/isolate.cc
Insertions: 2, Deletions: 15.
@@ -577,6 +577,8 @@
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 @@
Dart_Port control_port_id = isolate_control_port.Id();
- if (!PortMap::IsAcquirable(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,14 +709,6 @@
UNREACHABLE();
}
} else {
- if (!PortMap::IsAcquirable(control_port_id)) {
- const auto& error =
- String::Handle(String::New("Isolate has a message loop running"
- " or otherwise unavailable."));
- Exceptions::ThrowStateError(error);
- UNREACHABLE();
- }
-
if (isolate != nullptr) {
ASSERT(Thread::Current()->isolate() == isolate);
Thread::ExitIsolate(/*isolate_shutdown=*/false);
```
```
The name of the file: runtime/vm/message_handler.h
Insertions: 0, Deletions: 1.
@@ -146,7 +146,6 @@
void PostMessage(std::unique_ptr<Message> message,
bool before_events = false) override;
- bool is_scheduled() { return pool_ != nullptr; }
virtual void set_is_scheduled() {}
private:
```
```
The name of the file: runtime/vm/port.cc
Insertions: 4, Deletions: 14.
@@ -244,6 +244,10 @@
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,20 +258,6 @@
return IsolateAcquireResult::SUCCESS;
}
-bool PortMap::IsAcquirable(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);
- return target_handler->isolate()->is_acquirable();
-}
-
#if defined(TESTING)
bool PortMap::HasPorts(MessageHandler* handler) {
Locker ml;
```
Change information
Commit message:
[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 <veg...@google.com>
Files:
- M runtime/lib/isolate.cc
- M runtime/vm/isolate.cc
- M runtime/vm/isolate.h
- M runtime/vm/message_handler.cc
- M runtime/vm/message_handler.h
- M runtime/vm/port.cc
- M runtime/vm/port.h
- M tests/ffi/threading_test.dart
Change size: M
Delta: 8 files changed, 22 insertions(+), 37 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Slava Egorov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages