dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.2k stars 1.57k forks source link

[vm/ffi] Introduce a `NativeCallable.blocking` #54554

Open nielsenko opened 9 months ago

nielsenko commented 9 months ago

Currently we have

https://api.dart.dev/stable/3.2.4/dart-ffi/NativeCallable/NativeCallable.listener.html enables async callbacks that return immediately upon calling and schedule the callback to be run on the Dart event loop of the right isolate. No return values can be returned.

However, it is often the case that a native callback expects to be synchronous in that the parameters parsed are really stack allocated values, yet we still need to ensure the callback is ferried to the correct isolate event loop on the dart side.

So I suggest to add the option to block the callback on the native side until completed in the correct isolate on the dart side. It is okay for my use-case to still restrict to functions returning Void.

Would it be possible to implement such behaviour? I envision something like:

    final completer = Completer<void>();

    late final NativeCallable<Void Function(Pointer<native_error> error)> callback;
    void done(Pointer<native_error> error) {
      try {
        if (error != nullptr) return completer.completeWithError(error.asDart);
        completer.complete();
      } finally {
        // Block native side until here, if callback was constructed with 
        // listener(done, sync: true).
        // Use a separate from close to also support streaming. 
        callback.releaseCaller();         
        callback.close(); 
      }
    }

    callback = NativeCallable<Void Function(Pointer<native_error> error)>.listener(done, sync: true);

    return completer.future;

Currently, if native_error is really an address of a stack allocated native object, this would fail.

mit-mit commented 9 months ago

cc @mkustermann

nielsenko commented 9 months ago

Been thinking a bit more about it.

Maybe it would be better to be more explicit about the synchronization. The listener method could take an optional Mutex and ConditionVariable (from @mraleph's native_synchronization package), and if those where passed:

  1. [Native] Aquire the mutex when a callback is about to happen
  2. [Native] Schedule the callback on the correct isolate event loop
  3. [Native] Wait on the condition variable to be notified
  4. [Dart ] Once run, it is the callbacks responsibility to call ConditionVariable.notify to signal done (similar to calling NativeCallable.close).
  5. [Native] Wakes up, freeing any stack allocated params, etc.

It would be okay for me, if this machinery was actually visible on the interface, as it make it easy to re-use the synchronization primitives across calls, and also make it very apparent to users that they are dabbling in dead-lock land.

mkustermann commented 9 months ago

We had discussions about this before. For example https://github.com/dart-lang/sdk/issues/52689#issuecomment-1597082294:

class NativeCallable {
  /// Allocates a native callback resource to receive calls to the returned native function.
  ///
  /// The native caller is blocked until the function result is available, at which point it's 
  /// passed back to the caller, which is then unblocked.
  /// If the native call happens during a Dart call-out to the native code from the same
  /// isolate which created the native callback, the execution may continue on the same 
  /// thread. Otherwise, that isolate will also be blocked until the code has run in
  /// the isolate which did create the native callback.
  ///
  /// Use [NativeCallback.close] to free the resource when no further calls are expected.
  external static (NativeCallback, Pointer<NativeFunction<T>>) createBlockingCallback<T extends Function>
      (@CorrespondingDartType("T") Function function);
}

There's some questions: Define behavior when the isolate doesn't exist anymore (crash, block forever, simply return dummy C value). We kind of avoided this problem by saying: The isolate has to ensure it stays alive as long as C code may invoke the callback.

There were also some concerns around deadlock possibilities, though it could be put into API docs that users have to use it with care.

/cc @liamappelbe

nielsenko commented 9 months ago

I guess the isolate will still be kept alive until the explicit close call. I just want to ensure the native-side call don't complete until the callback has run on the isolate event loop.

I find it is often the case that native code expects the callback they invoke to run synchronously, but from an arbitrary thread. This poses a challenge currently.

NativeCallable.isolateLocal will run synchronously, but on whatever thread is used for the call. NativeCallable.listener will be scheduled on the correct isolate event loop, but this is inherently async as the native callback merely schedules the dart callback.

So I end up having to wrap the native code in yet another layer of native code, to ensure the parameters passed to the callback survives until it actually runs, which is just a bit sad.

Personally I'm fine with undefined behaviour, so crash, block forever, etc. For my use-case it is enough to support void functions, so I guess there is no possible dummy C value to return.

nielsenko commented 9 months ago

Reading through https://github.com/dart-lang/sdk/issues/52689#issuecomment-1598484349 I guess :

Immediate, blocking: Blocks current thread, triggers event which runs Dart function in its originating isolate. May wait for async results too. Returns the result, then unblocks. (Cannot work for current isolate, since it would block handling the event.)

would serve my purpose, but actually I don't need the return value (so more like fire-and-forget), but I do need the native side to block, so that any Pointer<native_t> pointing to stack values, and passed as parameters to the callback are still valid when the callback is actually run on the event loop.

mkustermann commented 9 months ago

would serve my purpose, but actually I don't need the return value

Your described problem requires synchronous execution of a Dart callback on another thread. If we implement such a NativeCallable.createBlockingCallback, then we may as well support any C type as return type, no reason to restrict it to void.

nielsenko commented 9 months ago

Yes indeed. I would be very happy if it supports return types as well.

EBoehmecke commented 3 months ago

We are also in need of a 'NativeCallable.createBlockingCallback' function, so that our native code that runs the native callback inside a thread, waits for the dart callback to finish.

dcharkes commented 2 months ago

From a discussion with @HosseinYousefi.

There could be usages of NativeCallable.blocking where you call back in to the native code from a NativeCallable.blocking. And the native code might use thread-local storage. So when using NativeCallable.blocking we might want to have the thread that was the mutator thread be not the mutator thread, and have the thread that is blocked from native code be the mutator thread for the duration of of the synchronous blocking callback.