Usibility issue with Isolate.run wrt to over-capturing

[goderbauer]

Isolate.run is difficult to use because of #36983. When the closure provided to Isolate.run captures unnecessary stuff (because of that bug) that can't be send to an isolate, the user only sees this cryptic message:

[ERROR:flutter/lib/ui/ui_dart_state.cc(198)] Unhandled Exception: Invalid argument(s): Illegal argument in isolate message: (object extends NativeWrapper - Library:'dart:ui' Class: Path)
#0      Isolate._spawnFunction (dart:isolate-patch/isolate_patch.dart:399:25)
#1      Isolate.spawn (dart:isolate-patch/isolate_patch.dart:379:7)
#2      Isolate.run (dart:isolate:232:15)
#3      _MyHomePageState._applySepiaFilterInIsolate (package:isoimg/main.dart:100:45)

It is not at all obvious that this is because the closure provided was over-capturing.

IMO, this is a major usability problem for Isolate.run because it is easy to use in a way that triggers the problem (especially in async/await contexts) and the solution is not clear from the error message.

Example where I ran into this: https://github.com/goderbauer/scratchpad/blob/8d82e738a4307c65651dc854ec6437002f69d6ed/isoimg/lib/main.dart#L90-L92

To make it work, one has to define a static method to avoid over-capturing: goderbauer/scratchpad@e36c311

[goderbauer]

/cc @mkustermann @mraleph @mit-mit

[goderbauer]

@mit-mit Since this functionality is new, should we actually back this out and not ship it in 2.17 in its current state?

If not, at a minimum the docs for the functionality probably deserve a disclaimer explaining this behavior.

[goderbauer]

Ideally, the error message would also be more informative...

[jellynoone]

I agree with @goderbauer that the current state is sub-optimal, but I believe a way to compute a single function in an Isolate should be provided by the core.

Mostly, because of how difficult it is to handle all the edge cases and clean up the resources. Even the initial implementation here had one such problem, not to mention the solution Flutter's compute used before.

However, it might be worth reconsider changing the signature to be more inline with what compute does i.e. accept a parameter. I see on Gerrit in the initially feature addition the reason, not to do this in the first place was because a closure capturing the necessary parameters would suffice.

But if a solutions like the one described above by @goderbauer will have to be written to avoid over-capturing, the ease of use really feels more like a trap than a feature.

[dnfield]

From offline conversation, it sounds like the direction will be to prioritize #36983 which should make this problem less serious and avoid a harder to use API.

[mkustermann]

From offline discussion between @lrhn @mit-mit @mraleph about this and related issues:

• Fix over-capturing issue in the VM: sdk/issues/36983 (possibly by capturing this.<final-field> values instead of this)
• Allow developers to see how much data gets transferred between isolates: sdk/issues/48591
• More precise error message why object cannot be sent: sdk/issues/48592

And for now we'll remove Isolate.run() so it won't be included in the next stable release.

[mit-mit]

@lrhn landed a new iteration of the isolate.run api:
a5f25ee

It looks like the main usecase in #36983 was resolved.

@goderbauer can you take a look at this and see if you think we can move forward with alignment with Flutter APIs?

[goderbauer]

can move forward with alignment with Flutter APIs?

Can you remind me what the alignment plan was for this?

[dnfield]

I think there was a patch at some point to make compute just use Isolate.run, right?

[mit-mit]

Shall we go ahead with that alignment, @goderbauer @dnfield ?

[goderbauer]

Yeah, that sounds good to me. Does one of you want to send a PR? :)

[mit-mit]

@dnfield any chance you're able to create that PR? Lasse is tied up with Dart 3 core lib changes we have to get done next week weeks.

[mit-mit]

@goderbauer can this issue be closed now?