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

Allow usages of closures for ffi callbacks. #52689

Closed
mkustermann opened this issue Jun 13, 2023 · 24 comments
Closed

Allow usages of closures for ffi callbacks. #52689

mkustermann opened this issue Jun 13, 2023 · 24 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@mkustermann
Copy link
Member

Right now ffi callbacks have to be top-level functions. This creates major inconveniences, as the callback may need to access some state in dart. Which means that that state also has to be global state - which is not nice for encapsulation, testing, etc.

As part of the async callback implementation we use now dynamically allocated ffi trampolines. This opens up the possibility to remove the restriction that the callback has to be a top-level function. Instead we can allow closures:

main() async {
  final (a, b) = await invokeAsyncNativeOperation();
}

Future<(int, int)> invokeAsyncNativeOperation() async {
  final completer = Completer<(int, int)>();

  late RawNativeCallback callback;
  callback = RawNativeCallbacks<Void Function(Int64, Int8)>((int arg0, int arg1) {
    completer.complete((arg0, arg1));
    callback.close();
  });
  processAsynchronously(callback.nativeFunction);

  return completer.future;
}

@Native<Void Function(Pointer<...>)>()
void processAsynchronously(Pointer<...> callback);

(Compare this with how the code would need to be written if we only allow a top-level function)

The only requirement is that the closure given to the RawNativeCallbacks constructor has to have a static type (void Function(int, int) in this example) that is compatible with the native type function type (Void Function(Int64, Int8) in this example).

When the metadata is allocated it can then put a Dart_PersistentHandle referring to the closure into the metadata. When closing the callback the persistent handle could be freed.

@mkustermann mkustermann added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Jun 13, 2023
@mkustermann
Copy link
Member Author

/cc @liamappelbe @dcharkes

@mkustermann
Copy link
Member Author

We could then extend the API to have a RawNativeCallback(<function>, {bool isSync: ...}) parameter, thereby allowing closures in synchronous callbacks as well.

@lrhn
Copy link
Member

lrhn commented Jun 13, 2023

Would be a definite usability win. Do it!

Having to use a top-level function just begs for a pattern like: late int Function(int, int) _actualFunction; int delegatingFunction(int a, int b) => _actualFunction(a, b);, where you set the function to actually call, globally, and then passes the delegating function to the native code. (Or some core function plus dynamic data stored on the side.)

That pattern doesn't scale. You need either a static function+state variable per native function to call, which means no creating native callbacks in loops.
Or you need to multiplex/trampoline by having the static function take an extra argument, which is used to look up dynamic state in a map, and passing the (key) value of the argument to the native code too. Which also complicates the native code.

And it would allow zone-aware callbacks, if you can do Zone.current.bind(callback) before wrapping it.
In fact, if possible, I think all native callbacks should be zone-bound by default, just like Dart async callbacks. It's far too easy to forget it, and you get weird errors when code runs in a different zone than expected. (There could be a "no zone" version as well, for when performance is the only thing that matters.)

@mkustermann
Copy link
Member Author

mkustermann commented Jun 13, 2023

Doing this we could then either deprecate & remove Pointer.fromFunction() or we could make a desugaring for it:

int foo(int arg0, int arg1) { ... }

main() {
  final pointer = Pointer.fromFunction<Int32 Function(Int8, Int64)>(foo);
}

to

void foo(int arg0, int arg1) { ... }

late final foo#Int32#Int8#Int64 = RawNativeCallbacks<Int32 Function(Int64, Int8)>(foo, isSync: true).nativeFunction;

main() {
  final pointer = foo#Int32#Int8#Int64;
}

That would have same behavior as Pointer.fromFunction<Int32 Function(Int8, Int64)>(foo) - which also allocates the trampoline and only frees it on isolate shutdown.

@dcharkes
Copy link
Contributor

I like everything in this thread! 👍

As part of the async callback implementation we use now dynamically allocated ffi trampolines. This opens up the possibility to remove the restriction that the callback has to be a top-level function.

Yes! Using an extra int argument and a map to find the right function as we have been recommending is not pretty at all.

{bool isSync: ...}

Should we consider an enum CallbackType instead? With values sync, asyncVoid, that enables us to have the same API for other types of callbacks if we want to in the future. From Liams doc:

  • block and wait to enter until isolate is free (with return value)
  • (the others we already have or are not callback types)

So then that CallbackType would get a third option blocking.

@liamappelbe
Copy link
Contributor

Doesn't that end up with the same problems we talked about here #51350? The sync and async constructors should probably be different, in case the APIs diverge as we add more optional params. For example, the sync callbacks will need an exceptional return.

The eventual goal of unifying them in one type sounds good to me though.

@liamappelbe
Copy link
Contributor

I'll land the current async callback class as RawVoidCallback, so that we can add the more general RawNativeCallback that does this abstraction later, without it being a breaking change.

@lrhn
Copy link
Member

lrhn commented Jun 14, 2023

@liamappelbe I'm not seeing how this design would be different from the proposed RawVoidCallback, other than accepting more functions as argument to the constructor.
Which means that if we support function values as non-synchronous native callbacks, we should not introduce a new class, but just accept more arguments to RawVoidCallback.
(Unless that's only stoned at @dcharkes comment, not the original post. Then it makes sense.)

A function which can return a value to massive code would be different, and maybe need a new class to support it, or maybe it should just be different constructors on the same class.

@dcharkes
Copy link
Contributor

without it being a breaking change

The only question for a breaking change would be what the bool isSync or CallbackType callbackType would default to. If we now introduce RawNativeCallback for only async, making sync also use that API later and defaulting it to sync would be a breaking change.

My preference would be to introduce RawNativeCallback, but mark it as experimental. That way we can still decide to make the sync one the default when switching Pointer.fromFunction over. @mit-mit any opinions on whether async callbacks should be marked as experimental for the first release they will be in?

@mkustermann
Copy link
Member Author

Doesn't that end up with the same problems we talked about here #51350? The sync and async constructors should probably be different, in case the APIs diverge as we add more optional params. For example, the sync callbacks will need an exceptional return.

Not quite sure what you're referring to here. The fact that exceptionalReturn was made a positional instead of a named parameter was a mistake. We're not making this mistake in the new class.

Other than that one could even use the very same constructor with optional named parameters (be it bool, enum, ...).

We have our own language extension that ensures that the parameters agree (e.g. exceptional-return is set when we need it and it's set to what we expect, that dart type agrees with native type, that calling method (e.g. async) has void return type, ...)

What's going to be common among all those callback types is that they will need

  • to be allocated & freed (i.e. they are a resource) - so they have a constructor and a close() method
  • they expose a native Pointer<> representing the C function pointer that C code can call

Not sure if we ever going to need to expose more than that, so do we really need to make several classes?

@lrhn Would we want to prefer separate classes or rather one class?

That being said one could imagine various configurations for them:

  • kind of call: sync callback, async call that returns void, possibly blocking async call with return value, ...
  • single-shot: could e.g. auto-close after receiving one call
  • ...

@lrhn
Copy link
Member

lrhn commented Jun 14, 2023

Let's consider all the functionalities we expect to eventually want, so we don't have to redesign the APIs later.

We already have "synchronous" Dart functions, which can only call back into Dart, into the same isolate, during a call out to native code. They can return a value immediately. They also need to, so the Dart function can't meaningfully return a future.

Then we have these fire-and-forget "event" Dart functions, where any native code can call them, at any time, and eventually they're executed in the originating Dart isolate. There is no return value, and no way to pass a value back.
There is no way to know whether anyone is still listening at the other end of the call, since there is no return channel at all.
There is a need to close down the listening end, since it may keep the isolate alive. (It would be great if ports had a flag that kept them from keeping an isolate alive, and this callback handler could have a similar flag, then you could choose to have a callback that only responds while something else keeps the isolate alive.)

This doesn't cover the following use-cases for native function pointes to Dart functions:

  • A Dart function which can be called back during a single native call, but which performs an async computation.
  • A Dart function which can be called from anywhere, but still return a value. (This function can be async too, we'.)

The former case might just be unacceptable, since it would allow asynchronous events to happend during an otherwise synchronous Dart execution. It's a way to get waitFor using native code.
So, maybe just not allow that. Which narrows the problems we need to solve to calling a Dart function, possibly async, from anywhere, and get a result back later somehow

The two immediate approaches would be:

  • Blocking until a result comes back.
  • Passing a native callback function as an argument in the call, and call that when the Dart result is ready. The native code has to handle its own asynchrony.

Blocking is bad. If I do a native call from Dart code, that native call does an "event" callback to the same isolate, then it can't block, because that would prevent control coming back to the isolate, so the event would never be handled. Houston, we have a deadlock.

The more approachable approach is to pass the native value callback as a native function pointer (or whatever works, in case we want to pass a C++ method pointer?), and then let the Dart side call the callback when it has a value.
That's something we can do with the VoidCallback already, just do:

var cb = RawVoidCallback<Void Function(Int32, Pointer<NativeFunction<Void Function(Int32)>>)>(
   void (int arg, resultCallback) async {
      var result = await asyncFibonnaci(arg);
      resultCallback(result); // or how we call a native function pointer.
   });

We can provide helpers, but since we can't abstract over argument lists, we can only do that for a certain set
of argument lists.

  /// Creates a native pointer which calls the [function] from native code.
  /// 
  /// The native pointer takes an extra argument, which will be called with the result of [function]
  /// when the computation is done.
  /// If [maxCalls] is greater than zero, the native function stops working after [maxCalls] calls
  /// have been received.
  Pointer<NativeFunction<Void Function(T1, Pointer<NativeFunction<Void Function<R>>>)>>
    asyncFunction<R extends NativeType, T extends NativeTYpe>(
        @CorrespondingDartAsyncOrType<R Function(T1)>() FutureOr<Object?> Function(Never) function,
        {final int maxCalls = -1}) {
     var calls = 0;
     late RawVoidCallback<Void Function(T1, Pointer<NativeFunction<Void Function(Int32)>>)> cb;
     cb = RawVoidCallback((v1, callback) {
          if (maxCalls > 0 && ++calls >= maxCalls) cb.close();
          var result = await function(arg); // not actually type-sound, we need to know the Dart type corresponding to R.
          callback(result);
     });
    return cb.nativeFunction;
  }

(I'm sure we need to be much trickier than that, but that's the idea).

If so, I think the RawVoidCallback might be the only thing we really need, the rest can be built on top of that, and then it's just a matter of figuring out how to thread the static typing constraints through the calls.

@liamappelbe
Copy link
Contributor

Not quite sure what you're referring to here. The fact that exceptionalReturn was made a positional instead of a named parameter was a mistake. We're not making this mistake in the new class.

We have 2 distinct kinds of functions with 2 very different semantics (blocking vs non-blocking). They already require different constructor args, and it's very possible that we'll add more. Eg, as Lasse suggests, we could have a version of async callbacks that doesn't keep the isolate alive, which we'd probably do as a constructor flag that would only be applicable to async callbacks.

We'd have to document this constructor well, to capture all the combinations of args that are allowed, as well as documenting the behaviour differences. All these docs would be of the form "[blah blah] when isSync is true." or "When isSync is false [blah blah].". So I think it would be clearer and less error prone to just make them separate APIs (separate named constructors/static functions on the same class is fine). That way these differences would be self documenting.

This is the conclusion we came to in #51350. Rather than adding a flag to Pointer.fromFunction, we opted to add Pointer.asyncFromFunction.

I also like the idea of starting with RawVoidCallback and adding this abstraction later (probably by adding a RawNativeCallback interface that both the async void and sync classes implement). This would let users check what sort of callback they have with an is check. I can include that interface in the current CL if adding it later would be a breaking change somehow. We can still have constructors/static functions on the RawNativeCallback that delegate to the implementation class's constructors.

@mkustermann
Copy link
Member Author

mkustermann commented Jun 19, 2023

If so, I think the RawVoidCallback might be the only thing we really need, the rest can be built on top of that, and then it's just a matter of figuring out how to thread the static typing constraints through the calls.

Let me expand on that a bit:

Once we allow closures in callbacks (this issue) our synchronous callbacks that allow return values need to be allocated / freed. i.e. they become a resource. Using a class with the name RawVoidCallback isn't appropriate as it's not actually returning void.

Regarding the async callbacks with actual return values: One may execute the dart function in a temporary isolate (by invoking a closure with state) and return the result. (Some other solutions that we are unlikely to implement, but never say no: async callback that block, async callback that doesn't execute in event-loop cycle). Again here the name RawVoidCallback isn't appropriate.

So the question is whether we should then introduce more classes, or we choose a different name - e.g. RawNativeCallback. On the surface it seems all scenarios can be covered with the same interface: Constructor (allocate resources), Pointer get nativePointer , close() method.

(In other parts of language we prefer single classes, e.g. List to cover const, immutable, growable, non-growable implementations)

So I think it would be clearer and less error prone to just make them separate APIs (separate named constructors/static functions on the same class is fine).

This would let users check what sort of callback they have with an is check.

It would certainly be different constructors. But the question is whether it should be separate classes. One can start with a RawNativeCallback with different factory constructors, and in the future make more subclasses and make the constructors return different subclasses. One cannot go the other way around (once RawVoidCallback, RawSyncCallback, RawSyncTempIsolateCallback, ... are out there, there's no going back)

I'm not too opinionated, but I lean on having a single class except if we have an actual need for having separate ones (e.g. if classes need different properties/methods, realistic reason a user may need to use is check on them, ...).

@lrhn you still think RawVoidCallback is the right choice? If so, we can also stick with that.

@dcharkes
Copy link
Contributor

So the question is whether we should then introduce more classes, or we choose a different name - e.g. RawNativeCallback. On the surface it seems all scenarios can be covered with the same interface: Constructor (allocate resources), Pointer get nativePointer , close() method.

+1 I believe we should have different constructors but a single class.

The constructor we add now should then probably be asyncVoid or voidAsync on RawNativeCallback.

@lrhn
Copy link
Member

lrhn commented Jun 19, 2023

I still think it should be (expected future) use-case driven, so if the expected use-cases include allocating resources for any kind of function, uniformly, then I see no problem with having a single class representing that.
As long as users can tell which constructor to use, and what it does.

The typing will be tricky, because you're abstracting over parameter lists, but if your @CorrespondingDartType(...) hack works, it should help.

I still see async callbacks as significantly different from sync callbacks, whether void or not, because you'll always want to handle the future on the Dart side, which means the behavior is not entirely defined by the Dart function itself, but also by what happens after it returns. Which means that it will need a separate constructor, but not necessarily a separate "resource class".
But again, if you can filter on the static type of the function, so that nobody passes in a Future<int> Function(int) where a native Int32 Function(Int32) is needed, it shouldn't be a problem.

Or, consider separating the resource from the function, so you don't need to vary the nativeFunction getter, and do:

/// A native callback resource which listens for calls to a native function.
///
/// Creates a native callback by using one of the [createCallback] or [createAsyncCallback]
/// functions to link the call to a Dart function, which will eventually be run with the same
/// arguments, converted to Dart values. 
///
/// The result of the Dart function 
abstract class NativeCallback<T extends Function> {
  /// Allocates a native callback resource to receive calls to the returned native function.
  ///
  /// The native function type must be a `void` function type, and no value is returned 
  /// to the caller.
  /// The call will not happen immediately, instead the isolate which created the 
  /// native callback will run the callback function as a new event loop event.
  ///
  /// Use [NativeCallback.close] to free the resource when no further calls are expected.
  external static (NativeCallback, Pointer<NativeFunction<T>>) createVoidCallback<T extends Function>
      (@CorrespondingDartType("T") Function function);

  /// Allocates a native callback resource to receive calls to the returned native function.
  ///
  /// The native aller 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);

  /// Allocates a native callback resource to receive calls to the returned native function.
  ////
  /// The native caller is blocked until the function result is available, which means
  /// after the async Dart [function] has returned a future, and that future has completed
  /// with a value. If the future completes with an error, the error is reported as uncaught,
  /// and the native function call returns zero-bits at its expected type (usually NULL-pointer
  /// or zero-valued integer). 
  ///
  /// The Dart function is run in a *new* isolate, on a separate thread, as if by an `Isolate.run`
  /// from the isolate which created the native callback.
  ///
  /// Use [NativeCallback.close] to free the resource when no further calls are expected.
  external static (NativeCallback, Pointer<NativeFunction<T>>) createBlockingAsyncCallback<T extends Function>
      (@CorrespondingAsyncDartType("T") Function function);

  void close();
}

Maybe it's overkill, and not a problem because the type of the native function is always T get nativeFunction.

I'm worried that constructors can't restrict the type arguments passed to them, they can't have further constraints on the type arguments like a static function can, but since we can't limit the type in the Dart type system anyway, it's probably not a problem.

@liamappelbe
Copy link
Contributor

Ok, sounds like I'm outvoted on separating the classes.

Does it still make sense to call it RawNativeCallback, since we're scrapping the idea of leaning into the port style API? I think the original reasoning for including Raw in the name was that it was like a RawReceivePort, and we would later add a non-raw wrapper that looks more like a ReceivePort. If we're not planning to add a separate NativeCallback wrapper class, it probably makes sense to just call this one NativeCallback.

As for naming the constructors, from Lasse's example it sounds like createFooCallback is the naming pattern. Since we'd like to avoid sync vs async (to avoid confusion with the Dart keywords, which describe a different concept), I guess the real distinction is blocking vs non-blocking, or thread-safe vs thread-unsafe.

class NativeCallback<T extends Function> {
  // Sync vs async option.
  NativeCallback.createSyncCallback(@DartRepresentationOf('T') Function callback, {Object? exceptionalReturn});
  NativeCallback.createAsyncCallback(@DartRepresentationOf('T') Function callback);

  // Blocking vs non-blocking option.
  NativeCallback.createBlockingCallback(@DartRepresentationOf('T') Function callback, {Object? exceptionalReturn});
  NativeCallback.createNonBlockingCallback(@DartRepresentationOf('T') Function callback);

  // Thread safe vs thread unsafe option.
  NativeCallback.createThreadUnsafeCallback(@DartRepresentationOf('T') Function callback, {Object? exceptionalReturn});
  NativeCallback.createThreadSafeCallback(@DartRepresentationOf('T') Function callback);

  Pointer<NativeFunction<T>> get pointer;

  void close();
}

@dcharkes
Copy link
Contributor

I'm worried that constructors can't restrict the type arguments passed to them, they can't have further constraints on the type arguments like a static function can, but since we can't limit the type in the Dart type system anyway, it's probably not a problem.

We'll have custom logic for check the correspondence between Dart and C types anyway. So making that logic check for Future<int> instead of int would be easy.

@lrhn
Copy link
Member

lrhn commented Jun 20, 2023

If it's a constructor, I'd remove the create. My examples were suggesting static factory functions to allow restricting the type parameters allowed for each (you cannot restrict a constructor to a subset of the types allowed by the class, sadly).
That might not be practical, since the type argument is T extends Function> anyway, and we cant restrict to only Void Function(something) in the current type system. In that case using a constructor is fine.
And constructor always have an implicit create, so you don't need to write it.
Also no need to repeat Callback, the class name is part of constructor names, more than they are of static names, so

Not sure the createSyncCallback fits in here, if it doesn't have to allocate a resource that must be closed.
(In that case, if we still do want it here, I'd make it just a static function returning just a Pointer<NativeFunction<T>>, not a constructor, since it doesn't need a NativeCallback resource object allocated.)

Also not sure about the separation of the three concerns (immediate/delayed, blocking/non-blocking and must-be-same-thread/can-be-any-thread/must-not-be-same-thread), since I think they are linked, some do not make sense by themselves, and some do not make sense combined.

A native function can either be fire-and-forget or wait for a result (or even just wait for completion).

Waiting for completion/result can happen in two ways:
* Run everything on the same thread, until completed.
* Block and run everything on a separate thread, then unblock when the result is ready.

If you run everything in the same thread, you might be able to call back into the current isolate, the one which called into the native code doing the callback. You probably can't run code in other existing isolates. You may be able to synchronously spawn a new isolate, run the callback function in that, take the results back out, and shut the isolate down again, all using only the one thread.

That suggests, to me, that the main categories of callbacks could be: immediate or fire-and-forget.
An immediate function can return a result, or guarantee the code has run, and the fire-and-forget returns immediately, and something happens later in Dart code, somewhere.
The immediate can work in two ways: Blocking or non-blocking. Blocking allows waiting for other threads, but precludes using the current thread. Nonblocking means using only the current thread.
Then callbacks can be separated into whether they expect to run in their original isolate, or if we want to create/spawn a new isolate and run them in that. Which is obviously more expensive (but maybe an isolate can be created by the NativeCallback creation in anticipation of getting called into, especially if it can be reused for multiple calls. But if there are multiple calls from different threads, then it won't be thread safe.).

So, the combinations that make sense are:

  • Specific/other isolate callback:

    • Immediate, unblocking: Calls directly back into the current thread's isolate and runs Dart code on top of the stack which contains existing Dart code stacks for that isolate. Only works for callbacks created by that isolate. (Checks that it runs in the same thread as the isolate it wants to call back into, fails if not.)
    • 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.)
    • Fire-and-forget: Triggers event which runs Dart function in its originating isolate (what started this issue).
  • New isolate callback:

    • Immediate: Synchronously spawns a new isolate from the originating isolate, runs the function in that, gets the (non-async) result, shuts down the isolate, returns the result. All on the same stack.
    • Immediate blocking: Blocks. Spawns new isolate in new thread, runs function in it, wait for result (may event be async). When ready, shut down isolate with result, and unblock.
    • Fire-and-forget: Triggers event to later spawn a new isolate, run the Dart function in it, and shut down the isolate normally when there's nothing more for it to do.

I might go with the following combinations:

/// A native callback resource which listens for calls to a native function.
///
/// Links a native function to to a Dart function, so that calling the native function will
/// call the Dart function in some way, with the same arguments as converted to Dart values. 
///
/// Some callbacks return values as well, others do not.
///
/// Some callbacks must be called on the same thread that the isolate creating it is running on,
/// others must not. Some work on any thread, either by not waiting for a result, or by blocking
/// the current thread if necessary.
///
/// The native callback allocates resources link Dart functions to native functions, and to
/// receive events from other isolates or threads. Those _must_ be disposed by calling 
/// [close], when no more callbacks are expected.
/// After calling [close], no further calls to the native function will succeed.
/// Blocking calls *may block indefinitely* on a closed native callback, 
/// so it's important to manage access to callbacks, and to not close the callback too early.
///
/// The [T] must be a native function type, like `Void Function(Int32)`.
class NativeCallback<T extends Function> {
  // If the first one needs to allocate resources to handle closure callbacks, just make it a constructor too.

  /// Must only be called in native code called to by the same isolate which created the callback.
  ///
  /// The return value of [callback] is directly sent back to the calling native code. 
  /// Therefore the return value cannot be a future.
  ///
  /// If the Dart [callback] function throws, the error is recorded as an uncaught error
  /// and the [exceptionalReturn] value is returned instead.
  /// If [exceptionalReturn] is omitted or `null`, it defaults to a zero-value of the corresponding 
  /// native type (a NULL-pointer, zero number value or false boolean).
  external static Pointer<NativeFunction<T>> immediate<T extends Function>(
      @DartRepresentationOf('T') 
      Function callback,
     {@DartRepresentationOfReturnTypeOf('T') 
      Object? exceptionalReturn,
     });

  /// Creates a native callback whose [nativeFunction] requests an execution of [callback].
  ///
  /// The native function type [T] must be a `void` function, like `Void Function(Int32)`.
  ///
  /// Native code on any thread can invoke the [nativeFunction], which immediately returns
  /// to the native code execution.
  /// At some later point, as an event, the isolate which originally created the native callback
  /// will execute the [callback] function. It's return value is ignored. Any errors thrown,
  /// immediately or asynchronously, become uncaught errors in that isolate.
  // TODO: run the function in the current zone? Yes, please, if possible.
  external NativeCallback.delayed(@DartRepresentationOf('T') Function callback);
  
  /// Creates a callback which may be called from any thread.
  ///
  /// If called from the thread of the same isolate which created the native callback,
  /// it will work like [NativeCallback.immediate], and calls directly back into the isolate.
  ///
  /// If not, the thread calling the native function will block, then trigger a later invoacation
  /// of the callback in its originating isolate with the provided arguments. 
  /// When that synchronous invocation is complete, the result is sent back, and the calling 
  /// thread is unblocked again with that return value.
  ///
  /// If the Dart [callback] function throws, the error is recorded as an uncaught error
  /// and the [exceptionalReturn] value is returned instead.
  /// If [exceptionalReturn] is omitted or `null`, it defaults to a zero-value of the corresponding 
  /// native type (a NULL-pointer, zero number value or false boolean).
  NativeCallback.mayBlock(
      @DartRepresentationOf('T') 
      Function callback, 
     {@DartRepresentationOfReturnTypeOf('T') 
      Object? exceptionalReturn,
     });
  
  /// Creates a callback which calls an async Dart function from another isolate.
  ///
  /// The callback may have a `Future` or `FutureOr` return type, and if it returns
  /// a future, that future will be awaited before the result is returned.
  ///
  /// If called on the thread of the isolate which created the native callback,
  /// the callback will fail and return the [exceptionalReturn] value.
  ///
  /// When called from another thread, the thread is blocked,
  /// and it triggers a later execution of the [callback] function in the isolate
  /// which created the native callback, with the provided arguments.
  /// When that computation completes with a result value, waiting for a future if necessary,
  /// the calling thread is unblocked with that return value.
  ///
  /// If the Dart [callback] function throws, the error is recorded as an uncaught error
  /// and the [exceptionalReturn] value is returned instead.
  /// If [exceptionalReturn] is omitted or `null`, it defaults to a zero-value of the corresponding 
  /// native type (a NULL-pointer, zero number value or false boolean).
  external NativeCallback.blockingAsync(
      @DartAsyncOrRepresentationOf('T')  // Same as DartRepresentationOf, except for `FutureOr` on return.
      Function callback,
    {@DartRepresentationOfReturnTypeOf('T') 
      Object? exceptionalReturn,
     });
 
  /// Creates a callback which calls an async Dart function in a new isolate.
  ///
  /// The callback may have a `Future` or `FutureOr` return type, and if it returns
  /// a future, that future will be awaited before the result is returned.
  ///
  /// Calling the native function will block the current thread, then 
  /// *spawn a new isolate* from the isolate which created the native callback, 
  /// and run the [callback] in that isolate (similarly to [Isolate.run]).
  /// When that computation completes with a result value, waiting for a future if necessary,
  /// the isolate is shut down again, and the calling thread is unblocked with that return value.
  ///
  /// If the Dart [callback] function throws, or the new isolate exits preamturely, 
  /// the error is recorded as an uncaught error and the [exceptionalReturn] value is returned instead.
  /// If [exceptionalReturn] is omitted or `null`, it defaults to a zero-value of the corresponding 
  /// native type (a NULL-pointer, zero number value or false boolean).
  external Native.blockingNewIsolateAsync(
      @DartAsyncOrRepresentationOf('T')
      Function callback,
    {@DartRepresentationOfReturnTypeOf('T') 
      Object? exceptionalReturn,
     });

  // We can have an `blockingOtherOrNewIsolate` which runs a callback in another isolate,
  // but if trying to do it in the same isolate, it spawns a new one. Probably too confusing.

  /// The native function which can be used to invoke the `callback` passed to the constructor.
  Pointer<NativeFunction<T>> get nativeFunction;
   
  /// Closes the native callback for further invocations and releases its resources.
  ///
  /// Existing running invocations are not affected.
  ///
  /// The [nativeFunction] *must not* be called after the native callback has been closed.
  /// Later invocations of the [nativeFunction] may do nothing, may crash, may block its thread
  /// indefinitely, may return a default value, and may invoke a completely different function.
  void close();
}

Some of these require the native function to have some leading logic which checks whether the isolate
it wants to invoke is the same that called it (if any), or not. More precisely, it needs to ensure it doesn't block the thread which runs the event loop of the isolate it wants to send an event to.
The "block and send event if other isolate, call sync if same" one might be tricky, depending on how much control you have over the native code stubs you generate for the nativeFunction, but it's basically just the same code paths as immediate plus what you'd do for an unonditional blocking constructor, and it avoids the error-case of blocking the current isolate forever.

@liamappelbe
Copy link
Contributor

Also not sure about the separation of the three concerns (immediate/delayed, blocking/non-blocking and must-be-same-thread/can-be-any-thread/must-not-be-same-thread), since I think they are linked, some do not make sense by themselves, and some do not make sense combined.

It's late here, so I'll respond to the rest of this tomorrow. Just wanted to clarify that my example only has 2 constructors, but I was presenting 3 different pairs of naming options.

@lrhn
Copy link
Member

lrhn commented Jun 20, 2023

Ah, that makes sense. In that case I might just go with .immediate and .delayed, if those are the only two constructors we'd ever need.

But I'd consider if we want more constructors later, and make sure we can distinguish all the use-cases that we want. If we want two different "delayed" versions, we might want to name them fooDelayed and barDelayed and not have one ambiguously called just delayed.

So, prepare for everything we can imagine! 😅

@liamappelbe
Copy link
Contributor

Not sure the createSyncCallback fits in here, if it doesn't have to allocate a resource that must be closed.

If we want to support closures in these callbacks (the topic of this bug), then we'll need a close method to clean up the closures. They'll also be cleaned up on isolate shutdown, but there will be cases where waiting for the isolate to shutdown will cause the number of active callbacks to grow unbounded.

Ah, that makes sense. In that case I might just go with .immediate and .delayed, if those are the only two constructors we'd ever need.

Hmmm. I like "immediate", but "delayed" makes me think of Future.delayed, so implies some sort of fixed duration delay. It's hard to find good synonyms for "async" that don't have strange implications, so maybe we should just go with "async". It's probably the most accurate, all things considered, and making the other option "immediate" rather than "sync" helps reduce confusion with the sync/async keywords.

But I'd consider if we want more constructors later, and make sure we can distinguish all the use-cases that we want.

In that case let's call include "void" in the name.

class NativeCallback<T extends Function> {
  NativeCallback.immediate(@DartRepresentationOf('T') Function callback, {Object? exceptionalReturn});
  NativeCallback.asyncVoid(@DartRepresentationOf('T') Function callback);

  Pointer<NativeFunction<T>> get pointer;

  void close();
}

@dcharkes
Copy link
Contributor

Discussion w @mkustermann @mraleph @lrhn @HosseinYousefi @liamappelbe and me:

  • NativeCallable abstract class with factory constructors
    • listener
    • isolateLocal
    • ("blocking" -> this one blocks if you're on a thread that has not entered the isolate. It has a fast path to isolateLocal.)

doc for isolateLocal:

/// Creates native function calling directly back into this isolate.
///
/// The native function must only be called from the same thread
/// that the isolate which created this native callback called out to native code on.

@liamappelbe
Copy link
Contributor

class NativeCallable<T extends Function> {
  NativeCallable.isolateLocal(@DartRepresentationOf('T') Function callback, {Object? exceptionalReturn});
  NativeCallable.listener(@DartRepresentationOf('T') Function callback);

  Pointer<NativeFunction<T>> get pointer;

  void close();
}

@lrhn
Copy link
Member

lrhn commented Jun 22, 2023

Make it

  Pointer<NativeFunction<T>> get nativeFunction;

and I'm happy 😀

(Could there be a

@DartRepresentationOfReturnTypeOf('T')

that could be applied to exceptionalReturn?)

@liamappelbe liamappelbe self-assigned this Jul 23, 2023
copybara-service bot pushed a commit that referenced this issue Jul 26, 2023
This change is almost trivial. The closure is stored on the callback's
RawReceivePort, not in the VM. So we can basically just remove the CFE
check and it pretty much works. The only problem is that we can't set
function.FfiCallbackTarget anymore, so most of the CL is dealing with
that.

A few places were deciding whether an FFI trampoline was a call or a
callback based on whether function.FfiCallbackTarget() was null. But
now the target will be null for async callbacks. So instead I've added
a new value to the FfiCallbackKind enum (and renamed it), and changed
those checks.

Sync callback closures will be a separate CL, because they're more
complicated.

Bug: #52689
Change-Id: I8e5dfb557362e679f66195b735c3c382e6792840
TEST=async_void_function_callbacks_test.dart
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316160
Commit-Queue: Liam Appelbe <liama@google.com>
Reviewed-by: Daco Harkes <dacoharkes@google.com>
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, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

4 participants