Async void ffi callbacks API

[liamappelbe]

We're adding a variant of Pointer.fromFunction that can be used for async void functions (by dispatching a message to the target isolate). We might also add more variants in future.

We could add an enum, as an optional parameter to Pointer.fromFunction that controls the variant.

enum FunctionPointerKind {
  sync,
  asyncVoid,
}

external static Pointer<NativeFunction<T>> fromFunction<T extends Function>(
    @DartRepresentationOf('T') Function f,
    [Object? exceptionalReturn,
    FunctionPointerKind kind = FunctionPointerKind.sync]);

This is nice because we can document all the different kinds of callback on this enum. The downside is that asyncVoid functions will always pass null as the exceptionalReturn, so until we add more variants, if you want to pass a non-default value to the third param you have to pass null to the second.

Another option is to add a new method for each variant:

external static Pointer<NativeFunction<T>> fromFunctionAsyncVoid<T extends Function>(
    @DartRepresentationOf('T') Function f);

A final option is to do a breaking change to switch Pointer.fromFunction to use named optional args.

enum FunctionPointerKind {
  sync,
  asyncVoid,
}

external static Pointer<NativeFunction<T>> fromFunction<T extends Function>(
    @DartRepresentationOf('T') Function f,
    {Object? exceptionalReturn,
    FunctionPointerKind kind = FunctionPointerKind.sync});

@dcharkes @lrhn any opinions?

[dcharkes]

I really prefer the last option, but we can't just break the API I'm afraid.

The best we can do is to deprecate fromFunction and introduce a new one:

class CallbackSemantics {
  CallbackSemantics._();

  /// documentation
  static const CallbackSemantics alreadyEnteredCorrectIsolate = ...;

  /// documentation
  static const CallbackSemantics asyncVoid = ...;
}

external static Pointer<NativeFunction<T>> fromFunctionV2<T extends Function>(
    @DartRepresentationOf('T') Function f,
    {Object? exceptionalReturn,
    CallbackSemantics kind = CallbackSemantics.alreadyEnteredCorrectIsolate});

fromFunctionV2 is a horrible name. Suggestions are welcome.

Please note we can't use an enum, as users might write a switch on it so adding a value later would break their code. Instead we should use a class with a private constructor and predefined const objects.

[blaugold]

Maybe from fromCallback or fromCallbackFunction?

[lrhn]

Edit: I should have read the documentation of fromFunction first.

I'd consider creating a separate function:

external static Pointer<NativeFunction<T>> fromAsyncFunction<T extends Function>> {
  @DartRepresentationOf('T') Function f,  // Must have a `Future<R>` return type.
  {Object? exceptionalReturn});

Making this a separate function makes it easier to diverge the APIs in the future, if necessary,
say if you want to add a timeout argument or something similar.
It also allows you to check that f returns a Future<R> for some R and that exceptionalReturn has a value assignable to that R (and that the Dart R type corresponds to the native return type of T).

This is different from the type checking on fromFunction, and things get easier when you keep them apart.
At least error recovery is easier: fromFunction in a function returning a Future<X>, that's just an error. You don't have to worry about whether the error is that it should be X or that the enum argument is missing.

Mode flags on functions that fundamentally change what the function does, is a design pitfall.
Initially it looks like you can save API surface and maybe reuse some of the code.
But it also makes the API harder to use and harder to evolve.
You are more likely to have to add later parameters which only apply to one of the modes.
Or you might want the same parameter name to apply to both modes, but with different types.
And if on of the modes would like to change its parameter types, maybe to something more permissive, the
other one will also have to be ready for that input, and might have to reject it with a runtime error.
Having to pass a mode flag, typically an enum or bool, usually means that one of the values is a default, so you only have to pass a flag in half the cases. It's not always clear which one should be the default.
And it's always verbose.

If instead you have two different functions, they can have different parameters, different type constraints, and none of them have to be the default.
It's cleaner.

Making exceptionalReturn named is fine (long word, though, I'd consider onError or errorValue, if you don't want to retain it for consistency).

[dcharkes]

Using enums (or any mode flags on methods) is ugly, verbose and forces multiple operations to have to share the same API. Here it doesn't look like the APIs differ, but are we sure they won't diverge in the future (say, adding an optional timeout to the async variant).

That's a good question, will the APIs ever diverge? If they don't diverge, having two identical parameter-lists is suboptimal. If they do diverge, only being able to throw ArgumentErrors at runtime is suboptimal. (This is why I keep tagging Lasse on these questions 😄 ! Thanks @lrhn!)

I can't come up with an idea why they would diverge just yet.

Mode flags on functions that fundamentally change what the function does, is a design pitfall.

Fundamentally it will always turn a Dart function into an address that a C function can call.
The thing that changes here is the threading logic: either the caller must be entered into the isolate already, or not.
I am not sure that qualifies as a different mode.

Thoughts @liamappelbe @mkustermann @mraleph ?

How is a native function "async" in the Dart sense (which is defined in terms of a Dart isolate's event loop)?

It is async in the sense that an arbitrary thread that doesn't respect (or know of) the Dart isolate's event loop can call it at an arbitrary point in time. Maybe "async" isn't the right term to use.

A try for better naming: runImmediate and scheduleEvent?

class CallbackSemantics {
  CallbackSemantics._();

  /// The calling thread must have already entered the correct isolate.
  ///
  /// By entering the Dart isolate in which this callback lives, the caller
  /// guarantees that no Dart code is currently executing.
  ///
  /// FFI callbacks with [runImmediately] semantics run synchronously and can return a value.
  static const CallbackSemantics runImmediately = ...;

  /// An FFI callback with [scheduleEvent] semantics can be invoked from any thread.
  ///
  /// FFI callbacks with [scheduleEvent] semantics are scheduled on the event loop.
  ///
  /// FFI callbacks with [scheduleEvent] semantics do _not_ deep copy their native arguments.
  /// The caller must only pass arguments by value, or pass ownership of memory referenced by [Pointer]s to the callee.
  ///
  /// FFI callbacks with [scheduleEvent] can only return `void` or `Future`s (which are exposed as `Dart_Handle`s).
  /// Callers with functions returning a `Future` in a `Dart_Handle` must have a handle scope.
  static const CallbackSemantics scheduleEvent = ...;

  /// Blocks the thread until the Dart code in the target isolate has yielded to the event loop.
  ///
  /// **Warning: using this can create deadlocks.**
  static const CallbackSemantics waitToEnter = ...;
}

Future<R>

@liamappelbe should we consider supporting functions that return a Dart_(Persistent)Handle with async callbacks? Those would be "async object" instead of "async void". For a Dart_Handle, the caller would need a handle scope.

[liamappelbe]

should we consider supporting functions that return a Dart_(Persistent)Handle with async callbacks?

That would be cool to have, but we should wait for a use case.

Lasse has convinced me that we should just add a separate function for this. Let's go with Pointer.fromAsyncFunction rather than AsyncVoid, so we can add support for returning Futures later. (I was saving that name for the wait-to-enter version, but on second thought that's probably a misnomer).

[dcharkes]

Pointer.fromAsyncFunction this is a misnomer if the Dart function we're making a native-function-pointer from is not async. It's the FFI trampoline that's async, not the Dart function. Or maybe one could say the native-function-pointer supports async calls. Maybe Pointer.asyncFromFunction then?

The CallbackSemantics argument would be easier with naming. (But we can restart that discussion for #51383 later.)

[liamappelbe]

Settled on NativeCallable.listener.