From 00cda4d60fa6987a5ed2768b26daabc66ce89a60 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Tue, 5 May 2026 08:37:06 -0700 Subject: [PATCH] [ffi/isolate_group] Move IsolateGroupBound callbacks from isolate to isolate group. This allows creation of isolategroup-bound callbacks when isolate is not available. For example, to support proposed Isolate::onEvent/Isolate::handleEvent api https://github.com/dart-lang/language/blob/main/working/333%20-%20shared%20memory%20multithreading/shared_native_memory.md#additional-isolate-apis. BUG=https://github.com/dart-lang/sdk/issues/63291 TEST=isolate_group_bound_callback_test CoreLibraryReviewExempt: vm ffi-specific comments changes Change-Id: I9a757a39ba8e152db64f59a66d337ca672103277 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/499661 Reviewed-by: Ryan Macnak Commit-Queue: Alexander Aprelev --- runtime/lib/ffi.cc | 9 +++- runtime/vm/bootstrap_natives.h | 1 + runtime/vm/isolate.cc | 23 +++++++--- runtime/vm/isolate.h | 12 +++-- sdk/lib/_internal/vm/lib/ffi_patch.dart | 45 +++++++++++++------ sdk/lib/ffi/ffi.dart | 7 ++- .../isolate_group_bound_callback_test.dart | 37 +++++---------- 7 files changed, 82 insertions(+), 52 deletions(-) diff --git a/runtime/lib/ffi.cc b/runtime/lib/ffi.cc index f55faac2250..72a75f7d485 100644 --- a/runtime/lib/ffi.cc +++ b/runtime/lib/ffi.cc @@ -52,7 +52,8 @@ DEFINE_NATIVE_ENTRY(Ffi_createNativeCallableIsolateGroupBound, 1, 2) { Function::CheckedHandle(zone, arguments->NativeArg0()); const auto& target = Closure::CheckedHandle(zone, arguments->NativeArgAt(1)); return Pointer::New( - isolate->CreateIsolateGroupBoundFfiCallback(zone, trampoline, target)); + thread->isolate_group()->CreateIsolateGroupBoundFfiCallback( + zone, trampoline, target)); } DEFINE_NATIVE_ENTRY(Ffi_deleteNativeCallable, 1, 1) { @@ -61,6 +62,12 @@ DEFINE_NATIVE_ENTRY(Ffi_deleteNativeCallable, 1, 1) { return Object::null(); } +DEFINE_NATIVE_ENTRY(Ffi_deleteIsolateGroupNativeCallable, 1, 1) { + const auto& pointer = Pointer::CheckedHandle(zone, arguments->NativeArg0()); + thread->isolate_group()->DeleteFfiCallback(pointer.NativeAddress()); + return Object::null(); +} + DEFINE_NATIVE_ENTRY(Ffi_updateNativeCallableKeepIsolateAliveCounter, 1, 1) { const int64_t delta = Integer::CheckedHandle(zone, arguments->NativeArg0()).Value(); diff --git a/runtime/vm/bootstrap_natives.h b/runtime/vm/bootstrap_natives.h index ab38f4db492..2ca92bc364d 100644 --- a/runtime/vm/bootstrap_natives.h +++ b/runtime/vm/bootstrap_natives.h @@ -302,6 +302,7 @@ namespace dart { V(Ffi_createNativeCallableIsolateLocal, 3) \ V(Ffi_createNativeCallableIsolateGroupBound, 2) \ V(Ffi_deleteNativeCallable, 1) \ + V(Ffi_deleteIsolateGroupNativeCallable, 1) \ V(Ffi_updateNativeCallableKeepIsolateAliveCounter, 1) \ V(Ffi_dl_open, 1) \ V(Ffi_dl_close, 1) \ diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index d49bce7dfe3..57671b616df 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -2705,6 +2705,13 @@ void Isolate::LowLevelCleanup(Isolate* isolate) { FinalizeWeakPersistentHandlesVisitor visitor(isolate_group); isolate_group->api_state()->VisitWeakHandlesUnlocked(&visitor); + // Clean up any synchronous FFI callbacks registered with this + // isolate group. Skip if this isolate group never registered any. + if (isolate_group->ffi_callback_list_head_ != nullptr) { + FfiCallbackMetadata::Instance()->DeleteAllCallbacks( + &isolate_group->ffi_callback_list_head_); + } + Thread::ExitIsolateGroupAsHelper(/*bypass_safepoint=*/false); } @@ -3817,16 +3824,20 @@ FfiCallbackMetadata::Trampoline Isolate::CreateIsolateLocalFfiCallback( &ffi_callback_list_head_); } -// TODO(aam): Should this be in IsolateGroup? -FfiCallbackMetadata::Trampoline Isolate::CreateIsolateGroupBoundFfiCallback( - Zone* zone, - const Function& trampoline, - const Closure& target) { +FfiCallbackMetadata::Trampoline +IsolateGroup::CreateIsolateGroupBoundFfiCallback(Zone* zone, + const Function& trampoline, + const Closure& target) { return FfiCallbackMetadata::Instance()->CreateLocalFfiCallback( - /*isolate=*/nullptr, group(), zone, trampoline, target, + /*isolate=*/nullptr, this, zone, trampoline, target, &ffi_callback_list_head_); } +void IsolateGroup::DeleteFfiCallback(FfiCallbackMetadata::Trampoline callback) { + FfiCallbackMetadata::Instance()->DeleteCallback(callback, + &ffi_callback_list_head_); +} + bool Isolate::HasLivePorts() { ASSERT(0 <= open_ports_ && 0 <= open_ports_keepalive_ && open_ports_keepalive_ <= open_ports_); diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index c436210afff..4ef258e7ce8 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -846,6 +846,12 @@ class IsolateGroup : public IntrusiveDListEntry { return thread_locals_count_.fetch_add(1u, std::memory_order_relaxed); } + FfiCallbackMetadata::Trampoline CreateIsolateGroupBoundFfiCallback( + Zone* zone, + const Function& trampoline, + const Closure& target); + void DeleteFfiCallback(FfiCallbackMetadata::Trampoline callback); + private: friend class Dart; // For `object_store_ = ` in Dart::Init friend class Heap; @@ -1008,6 +1014,8 @@ class IsolateGroup : public IntrusiveDListEntry { std::atomic thread_locals_count_ = 0; std::unique_ptr roots_; + + FfiCallbackMetadata::MetadataEntry* ffi_callback_list_head_ = nullptr; }; // When an isolate sends-and-exits this class represent things that it passed @@ -1338,10 +1346,6 @@ class Isolate : public IntrusiveDListEntry { const Function& trampoline, const Closure& target, bool keep_isolate_alive); - FfiCallbackMetadata::Trampoline CreateIsolateGroupBoundFfiCallback( - Zone* zone, - const Function& trampoline, - const Closure& target); void DeleteFfiCallback(FfiCallbackMetadata::Trampoline callback); void UpdateNativeCallableKeepIsolateAliveCounter(intptr_t delta); bool HasOpenNativeCallables(); diff --git a/sdk/lib/_internal/vm/lib/ffi_patch.dart b/sdk/lib/_internal/vm/lib/ffi_patch.dart index b5a8c54afa4..a46067ec83f 100644 --- a/sdk/lib/_internal/vm/lib/ffi_patch.dart +++ b/sdk/lib/_internal/vm/lib/ffi_patch.dart @@ -203,6 +203,11 @@ external void _deleteNativeCallable( Pointer pointer, ); +@pragma("vm:external-name", "Ffi_deleteIsolateGroupNativeCallable") +external void _deleteIsolateGroupNativeCallable( + Pointer pointer, +); + @pragma("vm:external-name", "Ffi_updateNativeCallableKeepIsolateAliveCounter") external void _updateNativeCallableKeepIsolateAliveCounter< NS extends NativeFunction @@ -348,28 +353,40 @@ final class _NativeCallableListener } final class _NativeCallableIsolateGroupBound - extends _NativeCallableBase { - bool _isKeepingIsolateAlive = true; + implements NativeCallable { + Pointer> _pointer; - _NativeCallableIsolateGroupBound(super._pointer) { - _updateNativeCallableKeepIsolateAliveCounter(1); + _NativeCallableIsolateGroupBound(this._pointer); + + @override + Pointer> get nativeFunction { + if (_isClosed) { + throw StateError("NativeCallable is already closed."); + } + return _pointer; } @override - void _close() { - _keepIsolateAlive = false; - } - - @override - void set _keepIsolateAlive(bool value) { - if (_isKeepingIsolateAlive != value) { - _isKeepingIsolateAlive = value; - _updateNativeCallableKeepIsolateAliveCounter(value ? 1 : -1); + void close() { + if (!_isClosed) { + _deleteIsolateGroupNativeCallable(_pointer); + _pointer = nullptr; } } @override - bool get _keepIsolateAlive => _isKeepingIsolateAlive; + void set keepIsolateAlive(bool value) { + if (value) { + throw ArgumentError( + "IsolateGroupBound callables can not keep isolate alive", + ); + } + } + + @override + bool get keepIsolateAlive => false; + + bool get _isClosed => _pointer == nullptr; } @patch diff --git a/sdk/lib/ffi/ffi.dart b/sdk/lib/ffi/ffi.dart index 4a0b7d64a7a..9c2f77180aa 100644 --- a/sdk/lib/ffi/ffi.dart +++ b/sdk/lib/ffi/ffi.dart @@ -414,8 +414,7 @@ abstract final class NativeCallable { /// _trivially shareable_. /// /// This callback must be [close]d when it is no longer - /// needed. An [Isolate] that created the callback will - /// be kept alive until [close] is called. + /// needed. /// /// After [NativeCallable.close] is called, invoking /// the [nativeFunction] from native code will cause @@ -522,6 +521,10 @@ abstract final class NativeCallable { /// By default, [NativeCallable]s keep the [Isolate] that created them alive /// until [close] is called. If [keepIsolateAlive] is set to `false`, the /// isolate may exit even if the [NativeCallable] isn't closed. + /// + /// Note: IsolateGroupBound-callbacks are not associated with an [Isolate], + /// so won't keep an isolate alive. Attempts to set [keepIsolateAlive] to + /// true, will throw. external bool get keepIsolateAlive; external set keepIsolateAlive(bool value); } diff --git a/tests/ffi/isolate_group_bound_callback_test.dart b/tests/ffi/isolate_group_bound_callback_test.dart index d0a8dc49646..92e31e01a4d 100644 --- a/tests/ffi/isolate_group_bound_callback_test.dart +++ b/tests/ffi/isolate_group_bound_callback_test.dart @@ -249,31 +249,18 @@ void testNativeCallableAccessNonSharedVar() { callback.close(); } -Future testKeepIsolateAliveTrue() async { - mutexCondvar = Mutex(); - conditionVariable = ConditionVariable(); - ReceivePort rpOnExit = ReceivePort("onExit"); - unawaited( - Isolate.spawn( - (_) async { - final callback = NativeCallable.isolateGroupBound( - simpleFunction, - ); - callback.keepIsolateAlive = true; - }, - /*message=*/ null, - onExit: rpOnExit.sendPort, - ), +Future testKeepIsolateAliveTrueThrows() async { + final callback = NativeCallable.isolateGroupBound( + simpleFunction, ); - try { - await rpOnExit.first.timeout(Duration(seconds: 5)); - // should not fall through, should throw TimeoutException - Expect.isTrue(false); - } catch (e) { - print('testKeepIsolateAliveTrue caught $e'); - Expect.isTrue(e is TimeoutException); - } - rpOnExit.close(); + Expect.throwsArgumentError(() { + callback.keepIsolateAlive = true; + }); + print(callback.nativeFunction); + callback.close(); + Expect.throwsStateError(() { + print(callback.nativeFunction); + }); } Future testKeepIsolateAliveFalse() async { @@ -312,7 +299,7 @@ main(args, message) async { testNativeCallableSync(); testNativeCallableSyncThrows(); testNativeCallableAccessNonSharedVar(); - await testKeepIsolateAliveTrue(); + await testKeepIsolateAliveTrueThrows(); await testKeepIsolateAliveFalse(); asyncEnd(); print("All tests completed :)");