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] Introduce a NativeCallable.blocking #54554

Open
nielsenko opened this issue Jan 9, 2024 · 12 comments
Open

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

nielsenko opened this issue Jan 9, 2024 · 12 comments
Labels
area-native-interop Used for native interop related issues, including FFI. area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug

Comments

@nielsenko
Copy link

nielsenko commented Jan 9, 2024

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
Copy link
Member

mit-mit commented Jan 10, 2024

cc @mkustermann

@mit-mit mit-mit added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Jan 10, 2024
@nielsenko
Copy link
Author

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
Copy link
Member

We had discussions about this before. For example #52689 (comment):

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
Copy link
Author

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
Copy link
Author

nielsenko commented Jan 11, 2024

Reading through #52689 (comment) 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
Copy link
Member

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
Copy link
Author

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

@a-siva a-siva added type-enhancement A request for a change that isn't a bug P3 A lower priority bug or feature request triaged Issue has been triaged by sub team labels Jan 11, 2024
@EBoehmecke
Copy link

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 dcharkes changed the title Feature request: Allow opt-in to native side blocking when invoking NativeCallable.listener callbacks [vm/ffi] Introduce a NativeCallable.blocking Jul 24, 2024
@dcharkes dcharkes added library-ffi area-native-interop Used for native interop related issues, including FFI. labels Jul 26, 2024
@dcharkes
Copy link
Contributor

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.

@dcharkes
Copy link
Contributor

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.

We can only solve this if we do thread pinning:

(In practice I don't believe the thread changes. Otherwise NativeCallable.isolateLocal and Pointer.fromFunction would be broken and might not return. But we don't officially provide the guarantee I believe.)

@mkustermann
Copy link
Member

We can only solve this if we do thread pinning:

That's orthogonal to this issue. Thread pinning means that an isolate is always going to run on one particular OS thread - even across event loop boundaries.

What's described here is that if a 3rd party thread calls into Dart (via NativeCallable.blocking) and that Dart calls back into native which calls back into Dart, that it's still on the 3rd party thread. That's the natural way a NativeCallable.blocking would be implemented and nothing special: The implementation would interrupt the running isolate, get it to a safepoint and then take ownership of the isolate and run the callback on the 3rd party thread. At that point the isolate runs on the 3rd party thread and can call native which can callback on the same thread. Once the outermost callback is done, it'd give ownership back to where it got it from, so the isolate would continue running the event loop on the thread(pool) where it would normally do.

(Side note: This is already possible today in a limited way: If the isolate voluntarily stops running on one thread (possibly with active stack) and continues running the isolate on another thread. What we don't have is a) explicitly stopping an isolate from the side (without cooperation / action by the isolate) b) expose a Dart API to make use of this for callbacks.)

Though taking a step back, I think we should align with the work on shared memory multithreading (/cc @mraleph @aam ) and see if we really want to expose a NativeCallable.blocking that executes code on the same isolate or whether cthe shared multithreading ability of shared isolates will be sufficient.
=> If possible we should mainly rely on this capability and avoid introducing anything else unless really needed.

@dcharkes
Copy link
Contributor

Ah, NativeCallable.blocking would be synchronous code only, which means no event loop boundaries, which means no thread switching?

That's the natural way a NativeCallable.blocking would be implemented

I also believe it would be the right way. The reason I remarked on this is because dart-lang/native#1796 uses the other semantics and I'd like to have the option to replace that implementation with this if possible in the future.

Though taking a step back, I think we should align with the work on shared memory multithreading (/cc @mraleph @aam ) and see if we really want to expose a NativeCallable.blocking that executes code on the same isolate or whether cthe shared multithreading ability of shared isolates will be sufficient.

👍

(That brings up an interesting question: If we have needs from JNIgen/FFIgen to do these kind of callbacks, would it solve those use cases to force those callbacks to be to shared isolates. We'd probably need go through an actual use case there to know the answer. E.g. What if some data needs to be main isolate / platform thread, which is not going to be shared.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-native-interop Used for native interop related issues, including FFI. area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants