Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vm/ffi] Calling NativeCallable.listener from a NativeFinalizer callback #54939

Closed
dcharkes opened this issue Feb 16, 2024 · 4 comments
Closed
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi P2 A bug or feature request we're likely to work on

Comments

@dcharkes
Copy link
Contributor

Currently, calling a NativeCallable.listener from a NativeFinalizer callback hits an assert:

../../runtime/vm/runtime_entry.cc: 4040: error: expected: current_thread->execution_state() == Thread::kThreadInNative

Isolate* current_isolate = nullptr;
if (current_thread != nullptr) {
current_isolate = current_thread->isolate();
ASSERT(current_thread->execution_state() == Thread::kThreadInNative);
current_thread->ExitSafepoint();
current_thread->set_execution_state(Thread::kThreadInVM);
}

The assumption is that if we have a current thread, it must be in native code. However, when the NativeCallable.listener is called from a finalizer callback, we have a current thread, but it is in the VM.

Repro: https://github.com/dart-lang/native/tree/native-finalizer/pkgs/native_assets_cli/example/native_add_library

@liamappelbe Could you please take a look at supporting this use case?

cc @mkustermann

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi labels Feb 16, 2024
@dcharkes dcharkes added the P2 A bug or feature request we're likely to work on label Feb 16, 2024
@liamappelbe
Copy link
Contributor

The real issue with this use case is a GC deadlock. I've investigated this before, as part of the ObjC Block finalizer flow:
dart-lang/native#204 (comment)
dart-lang/native#204 (comment)

At that time I decided it wasn't worth the effort to support this case, because I could just clean up the blocks using native code. Do we have a more compelling use case now?

@dcharkes
Copy link
Contributor Author

The use case is native finalizers which have an out parameter to communicate error messages:

So the issue here is that we need to run the argument unwrapping in a Dart isolate before it queues the message on the event loop.

The marshalling can also no be done in a new isolate group (with separate gc), because it could potentially instantiate Struct objects.

Some ideas:

  • NativeCallable.listenerNoIsolate which only allows passing integers. And then the user has to do Pointer.fromAddress. Or we somehow make Pointers sendable to other isolate groups (they are only an int anyway, as the type argument is always Never at runtime).
  • Let users use dart_api_dl.h with native ports for this use case. (Not very pretty.)

Maybe @mkustermann has some bright ideas on how we could enable using NativeFinalizers using NativeCallables.

@dcharkes
Copy link
Contributor Author

As discussed offline with @liamappelbe and @mkustermann:

  1. Resources should not be freed in Dart code. (Not the use case mentioned above.) When the isolate shuts down that would lead to resources not being freed.
  2. Errors should not be reported via a Dart callback. (The use case above.) That would swallow error messages if the isolate is shut down. Instead the errors should be reported via whatever native error reporting API is available. For example: https://developer.android.com/reference/android/util/Log and https://developer.apple.com/documentation/os/logging.

@dcharkes dcharkes closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2024
@mkustermann
Copy link
Member

Errors should not be reported via a Dart callback. (The use case above.) That would swallow error messages if the isolate is shut down. Instead the errors should be reported via whatever native error reporting API is available. For example: https://developer.android.com/reference/android/util/Log and https://developer.apple.com/documentation/os/logging.

The C API for android logging (similar to printf for normal programs) would be https://developer.android.com/ndk/reference/group/logging#__android_log_print

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

3 participants