From 82ff34d7751741fceb9cd1420519c6286c4201ce Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Wed, 27 Sep 2023 22:18:47 +0000 Subject: [PATCH] [ffi] Make nativeFunction throw if already closed This is a *breaking change*. See https://github.com/dart-lang/sdk/issues/53311 Make nativeFunction throw a StateError if the NativeCallable is already closed. Also make close and keepIsolateAlive not throw. I'm not adding an isClosed getter, as discussed on the bug. Bug: https://github.com/dart-lang/sdk/issues/53311 Fixes: https://github.com/dart-lang/sdk/issues/53311 Change-Id: Ib0066352d3cfc01d31df8ae8fd61be426fcdf6e1 CoreLibraryReviewExempt: The FFI package is VM-only TEST=async_void_function_callbacks_test and isolate_local_function_callbacks_test Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/328280 Reviewed-by: Daco Harkes Commit-Queue: Liam Appelbe --- CHANGELOG.md | 3 ++ sdk/lib/_internal/vm/lib/ffi_patch.dart | 41 ++++++++++++------- sdk/lib/ffi/ffi.dart | 6 +-- .../async_void_function_callbacks_test.dart | 13 ++++-- ...isolate_local_function_callbacks_test.dart | 17 ++++++-- 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec3fb099186..5c5fe10335b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,9 @@ constraint][language version] lower bound to 3.2 or greater (`sdk: '^3.2.0'`). - All `NativeCallable` constructors can now accept closures. Previously `NativeCallable`s had the same restrictions as `Pointer.fromFunction`, and could only create callbacks for static functions. +- **Breaking change** [#53311][]: `NativeCallable.nativeFunction` now throws an + error if is called after the `NativeCallable` has already been `close`d. Calls + to `close` after the first are now ignored. #### `dart:io` diff --git a/sdk/lib/_internal/vm/lib/ffi_patch.dart b/sdk/lib/_internal/vm/lib/ffi_patch.dart index 135bbb1b9ce..5d02b6a95e2 100644 --- a/sdk/lib/_internal/vm/lib/ffi_patch.dart +++ b/sdk/lib/_internal/vm/lib/ffi_patch.dart @@ -227,16 +227,34 @@ abstract final class _NativeCallableBase _NativeCallableBase(this._pointer); @override - Pointer> get nativeFunction => _pointer; + Pointer> get nativeFunction { + if (_isClosed) { + throw StateError("NativeCallable is already closed."); + } + return _pointer; + } @override void close() { - if (_pointer == nullptr) { - throw StateError("NativeCallable is already closed."); + if (!_isClosed) { + _deleteNativeCallable(_pointer); + _pointer = nullptr; } - _deleteNativeCallable(_pointer); - _pointer = nullptr; } + + @override + void set keepIsolateAlive(bool value) { + if (!_isClosed) { + _setKeepIsolateAlive(value); + } + } + + @override + bool get keepIsolateAlive => _isClosed ? false : _getKeepIsolateAlive(); + + void _setKeepIsolateAlive(bool value); + bool _getKeepIsolateAlive(); + bool get _isClosed => _pointer == nullptr; } final class _NativeCallableIsolateLocal @@ -252,13 +270,6 @@ final class _NativeCallableIsolateLocal } @override - void set keepIsolateAlive(bool value) { - if (_pointer == nullptr) { - throw StateError("NativeCallable is already closed."); - } - _setKeepIsolateAlive(value); - } - void _setKeepIsolateAlive(bool value) { if (_keepIsolateAlive != value) { _keepIsolateAlive = value; @@ -267,7 +278,7 @@ final class _NativeCallableIsolateLocal } @override - bool get keepIsolateAlive => _keepIsolateAlive; + bool _getKeepIsolateAlive() => _keepIsolateAlive; } final class _NativeCallableListener @@ -286,12 +297,12 @@ final class _NativeCallableListener } @override - void set keepIsolateAlive(bool value) { + void _setKeepIsolateAlive(bool value) { _port.keepIsolateAlive = value; } @override - bool get keepIsolateAlive => _port.keepIsolateAlive; + bool _getKeepIsolateAlive() => _port.keepIsolateAlive; } @patch diff --git a/sdk/lib/ffi/ffi.dart b/sdk/lib/ffi/ffi.dart index 756e493471e..ea01f64bdab 100644 --- a/sdk/lib/ffi/ffi.dart +++ b/sdk/lib/ffi/ffi.dart @@ -270,15 +270,15 @@ abstract final class NativeCallable { /// The native function pointer which can be used to invoke the `callback` /// passed to the constructor. /// - /// If this receiver has been [close]d, the pointer is a [nullptr]. + /// This pointer must not be read after the callable has been [close]d. Pointer> get nativeFunction; /// Closes this callback and releases its resources. /// /// Further calls to existing [nativeFunction]s will result in undefined - /// behavior. New accesses to [nativeFunction] will give a [nullptr]. + /// behavior. /// - /// This method must not be called more than once on each native callback. + /// Subsequent calls to [close] will be ignored. /// /// It is safe to call [close] inside the [callback]. void close(); diff --git a/tests/ffi/async_void_function_callbacks_test.dart b/tests/ffi/async_void_function_callbacks_test.dart index c755d1fd573..7ae637318f1 100644 --- a/tests/ffi/async_void_function_callbacks_test.dart +++ b/tests/ffi/async_void_function_callbacks_test.dart @@ -89,10 +89,15 @@ testNativeCallableDoubleCloseError() { final callback = NativeCallable.listener(simpleFunction); Expect.notEquals(nullptr, callback.nativeFunction); callback.close(); - Expect.equals(nullptr, callback.nativeFunction); + Expect.throwsStateError(() { - callback.close(); + final _ = callback.nativeFunction; }); + + // Expect that these do not throw. + callback.close(); + callback.keepIsolateAlive = true; + Expect.isFalse(callback.keepIsolateAlive); } Future testNativeCallableUseAfterFree() async { @@ -129,7 +134,9 @@ Future testNativeCallableNestedCloseCall() async { Expect.equals(1123, await simpleFunctionResult.future); // The callback is already closed. - Expect.equals(nullptr, simpleFunctionAndCloseSelf_callable!.nativeFunction); + Expect.throwsStateError(() { + final _ = simpleFunctionAndCloseSelf_callable!.nativeFunction; + }); } void simpleFunctionThrows(int a, int b) { diff --git a/tests/ffi/isolate_local_function_callbacks_test.dart b/tests/ffi/isolate_local_function_callbacks_test.dart index 9b49fc29965..5b582e52d9e 100644 --- a/tests/ffi/isolate_local_function_callbacks_test.dart +++ b/tests/ffi/isolate_local_function_callbacks_test.dart @@ -84,10 +84,15 @@ testNativeCallableDoubleCloseError() { exceptionalReturn: 0); Expect.notEquals(nullptr, callback.nativeFunction); callback.close(); - Expect.equals(nullptr, callback.nativeFunction); + Expect.throwsStateError(() { - callback.close(); + final _ = callback.nativeFunction; }); + + // Expect that these do not throw. + callback.close(); + callback.keepIsolateAlive = true; + Expect.isFalse(callback.keepIsolateAlive); } late NativeCallable selfClosingStaticCallback; @@ -105,7 +110,9 @@ testNativeCallableNestedCloseCallStatic() { callTwoIntFunction(selfClosingStaticCallback.nativeFunction, 1000, 234)); // The callback is already closed. - Expect.equals(nullptr, selfClosingStaticCallback.nativeFunction); + Expect.throwsStateError(() { + final _ = selfClosingStaticCallback.nativeFunction; + }); } testNativeCallableNestedCloseCallClosure() { @@ -122,7 +129,9 @@ testNativeCallableNestedCloseCallClosure() { Expect.equals(1234, callTwoIntFunction(callback.nativeFunction, 1000, 234)); // The callback is already closed. - Expect.equals(nullptr, callback.nativeFunction); + Expect.throwsStateError(() { + final _ = callback.nativeFunction; + }); } int throwerCallback(int a, int b) {