Listener/Async Blocks #601
Conversation
There was a problem hiding this comment.
LGTM.
Should we maybe add some integration test with a real world API that needs this?
(Nit, I stand by #363 (comment), I now had to review the generator instead of the generated API. 👓 )
| final thread = BlockTester.callOnNewThread_(lib, block); | ||
| thread.start(); | ||
|
|
||
| await hasRun.future; |
| @@ -591,7 +591,7 @@ class StringConfigSpec<RE extends Object?> extends ConfigSpec<String, RE> { | |||
| if (!o.checkType<String>(log: log)) { | |||
| return false; | |||
| } | |||
| if (_regexp != null && !_regexp!.hasMatch(o.value as String)) { | |||
| if (_regexp != null && !_regexp.hasMatch(o.value as String)) { | |||
There was a problem hiding this comment.
I changed this because when I bumped the SDK version the analyzer started complaining about it. My guess is that in the new SDK this field promotion is ok because it's a private field. The thing that makes field promotion impossible is that other libraries could override the getter in ways that would break the null safety inferences, but they can't do that if it's a private field.
There was a problem hiding this comment.
Better fix: #605
| /// This is based on FFI's NativeCallable.listener, and has the same | ||
| /// capabilities and limitations. This block can be invoked from any thread, | ||
| /// but only supports void functions, and is not run synchronously. See | ||
| /// NativeCallable.listener for more details. |
There was a problem hiding this comment.
This should probably have some documentation about not keeping the isolate alive.
Also, are there use cases in which users would want to keep the isolate alive? Should we surface a bool on the API?
There was a problem hiding this comment.
Comment updated.
We could do that, but note that ObjCBlock.listener doesn't have a 1:1 mapping with a NativeCallable.listener. All ObjCBlock.listener in an isolate use the same NativeCallable.listener trampoline. So it's not a simple matter of plumbing through that bool.
Instead we'd need to manually implement a way to keep the isolate alive (probably creating a RawReceivePort that we don't actually use). This is something that the user can already do if they want, but it's probably worth doing anyway, for ergonomics. Maybe we should do this for all blocks, not just listeners. But it's blocked on https://github.com/dart-lang/ffigen/issues/428 atm. I'll file a bug.
| @@ -13,7 +15,7 @@ topics: | |||
| - codegen | |||
|
|
|||
| environment: | |||
| sdk: ">=3.0.0 <4.0.0" | |||
| sdk: ">=3.2.0-114.0.dev <4.0.0" | |||
There was a problem hiding this comment.
What's the reason for the generator (ffigen) to require the new SDK version?
There was a problem hiding this comment.
Listener blocks are built on NativeCallable.listener. While that feature did land in 3.1, there are some important updates to it in 3.2 that we need (specifically keepIsolateAlive = false).
This reverts commit 416fccd.
Add support for listener blocks, built on
NativeCallable.listener. Also, bump the SDK version to 3.2 so we get thekeepIsolateAlivefield (otherwise the internal async trampoline will prevent the isolate from closing).All the substantive changes are in lib/src/code_generator/objc_block.dart.
Block.listeneruses all the same infrastructure asBlock.fromFunction, even using the exact sameregisterClosureandclosureTrampolinefunctions. The only difference is that listeners wrap theclosureTrampolinein aNativeCallable.listener, whereas ordinary Blocks wrap that same function in aPointer.fromFunction.