[breaking change] [vm/ffi] Closed NativeCallables nativeFunction should throw instead of return nullptr

[dcharkes]

Change Intent

abstract final class NativeCallable<T extends Function> {
  /// The native function pointer which can be used to invoke the `callback`
  /// passed to the constructor.
  ///
  /// If this receiver has been [close]d, the pointer is a [nullptr].
  Pointer<NativeFunction<T>> get nativeFunction;
}

->

abstract final class NativeCallable<T extends Function> {
  /// The native function pointer which can be used to invoke the `callback`
  /// passed to the constructor.
  ///
  /// Accessing this after this receiver has been [close]d is an error.
  Pointer<NativeFunction<T>> get nativeFunction; // Throws StateError after closed.
}

Justification

I wonder (maybe for another CL) whether better semantics would be: When getting nativeFunction pointer of a closed callable it will throw rather than silently returning nullptr - which Dart code may just give to C code, which then somewhere deep in the guts of some C code causes a crash?

@mkustermann in https://dart-review.googlesource.com/c/sdk/+/317060/20/sdk/lib/ffi/ffi.dart#225

cc @liama

Impact

I believe the impact to be minimal, because it's quite unlikely that nullptr is already used by people in native code to check that some native callable is closed. Also, native callable has only been in stable for a week.

Mitigation

Users can wrap the access in a try-catch and return nullptr (however one shouldn't catch StateErrors).

If we want to provide a cleaner solution we should maybe add a bool get closed to query whether the callable has been closed.

[dcharkes]

cc @lrhn as well

[parlough]

Did you mean to cc @liamappelbe instead of @liama? :)

[itsjustkevin]

@dcharkes a few things here:

1. As we go through this breaking chance process, can you add a section in the issue covering the CLs required for this change? This assists the tech writers in documenting changes in the future.
2. Is this change expected in the next release or a future release.
3. this reminds me that I should update the template and process for breaking changes 🙂.

@vsmenon @Hixie @grouma reaching out for breaking change review.

[dcharkes]

1. As we go through this breaking chance process, can you add a section in the issue covering the CLs required for this change? This assists the tech writers in documenting changes in the future.

This should be a fairly minimal change I believe. @liamappelbe

2. Is this change expected in the next release or a future release.

The next release.

[Hixie]

Seems reasonable.

[Piero512]

Hey, since we're doing this change, I would also add that the close function should also return silently if the Native Callable is already closed. (the alternative of throwing would probably work, but we don't want to encourage users to catch StateErrors).

[vsmenon]

As you say, we don't want to encourage people to catch an Error, so perhaps worth adding the closed getter as part of this. @lrhn @munificent for style / convention comments.

Otherwise, lgtm.

[lrhn]

If just let close silently do nothing of it's already closed.
If only add an isClosed getter if there is something useful to use it for. For sobering like this, you're not expected to share the controller, so you should know whether you've called close or not.

[itsjustkevin]

@grouma any thoughts on this change?

[grouma]

ACX doesn't make use of ffi so we are agnostic to this change. Looks like one customer has very minor usage and should not be impacted.

[liamappelbe]

EDIT: After discussions on the PR, decided to make the close method and keepIsolateAlive not throw.

I'm going to make keepIsolateAlive's getter/setter also throw a StateError if the NativeCallable is closed, for consistency (in fact isolateLocal's setter already throws).

I'm not making any changes to the close method (it will continue to throw), or adding an isClosed getter (as Lasse points out, we shouldn't add this without a use case).