Skip to content

Allow sending more kinds of objects between isolates #46623

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

Closed
5 tasks done
mkustermann opened this issue Jul 15, 2021 · 21 comments
Closed
5 tasks done

Allow sending more kinds of objects between isolates #46623

mkustermann opened this issue Jul 15, 2021 · 21 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-isolate

Comments

@mkustermann
Copy link
Member

mkustermann commented Jul 15, 2021

We would like to be able to loosen the current requirements on objects that can be sent between isolates through SendPorts. We can solve this once lightweight isolates are enabled by-default (see #36097) - which is slowly coming to its finishing line.

We want to allow sending:

  • Function types (formerly #44624, #44431)
  • Closures (formerly #40370)
  • Generic closures (formerly #40919)
  • StackTrace objects (no former issue, but lib_2/isolate/stacktrace_message_test fails)

Furthermore we want to:

  • Be less ambiguous about what can and cannot be sent across SendPorts.

Once lightweight isolate support (see #36097) has been enabled by-default (probably soon), we can easily add this support.

@abobija
Copy link

abobija commented Jul 18, 2021

Does anyone works on this? Is there any info when we will be able to send Function types?

@mkustermann
Copy link
Member Author

Does anyone works on this? Is there any info when we will be able to send Function types?

Yes, we are actively working on it. We are in preparation for enabling lightweight isolates by-default (see #36097). After that doing that, allowing more object types to be sent between isolates is a piece of cake.

@lrhn
Copy link
Member

lrhn commented Jul 26, 2021

If you can send closures, that will open up sending a lot of low-level classes, like Future and Zone (the reason you currently can't send a Future without listeners is that you can't send the Zone it contains).
Sending a Future is very likely to end up with a Future which never completes (you didn't send the completer, microtask or other future expecting to complete it).

If you send a stream pre-listening, it will include its controller and the onListen callback of that, which might risk accessing the same resource more than once.
Sending through a send port is extra-language cloning (creating an object without calling a constructor). Not all classes are suited for that. Would it be reasonable to introduce a "non-sendable" marker that you can put on classes?

There might be cases where you do want to send a primed stream to a worker isolate, to make it take over, as long as you don't use it locally, so maybe strictly making streams non-sendable isn't the best approach.
Maybe getting a "you've been cloned" callback to allow the class to disable itself would be better.

@mnordine
Copy link
Contributor

mnordine commented Aug 2, 2021

There might be cases where you do want to send a primed stream to a worker isolate

Sending a Stream to an Isolate would indeed be a nice use case. See similar thinking in the JS world (Transferrable Streams):

https://www.chromestatus.com/feature/5298733486964736#details
https://github.com/whatwg/streams/blob/main/transferable-streams-explainer.md

@mkustermann
Copy link
Member Author

Summary of meeting about sending closures (between @aam @lrhn and @mkustermann ):

  • Already today we can send Completers, Futures, StreamControllers and Streams (as long as no listeners are installed)
  • Already today we have some issues (e.g. we only re-hash VM-specific identity hashmaps/sets) - adding closure support doesn't change this.
  • We don't think allowing sending of closures will indirectly allow sending of objects that are very problematic

There's a question of how much state a local closure closes over. Right now the VM's implementation closes over more state than needed (an implementation choice) that can lead to hanging on to more objects than user code can access (though a debugger might be able to access them). See e.g. #36983. Allowing sending of closures may make this "memory leak" more prominent.

In general there's no strong reservations against loosing that restriction.

dart-bot pushed a commit that referenced this issue Sep 3, 2021
We already allow sending closures (if immutable via sharing
otherwise via copying). This CL makes use of this to allow the argument
to `Isolate.spawn()` to be any closure.

Similar to normal message sending, the spawn will fail if the closure
cannot be sent (or causes an error on the new isolate, e.g. rehashing
error).

Issue #46623

TEST=vm/dart{_2,}/isolates/closure_entrypoint_test

Change-Id: Iab342267d87bd87bc8c0c82d16aec58a69a3df44
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212295
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>
@mkustermann
Copy link
Member Author

As a small update: This has mostly been adressed (closures, function types, ... can be sent now across SendPorts). Sending StackTrace objects is remaining as is updated documentation.

@maks
Copy link

maks commented Sep 9, 2021

...One can use now closures in messages sent via SendPort as well as as entrypoint in Isolate.spawn()

Wow this is excellent news @mkustermann ! Is using closures as Isolate.spawn() entry point now also available in master?

@aam
Copy link
Contributor

aam commented Sep 9, 2021

@maks Yes it is. (it's main now)

copybara-service bot pushed a commit that referenced this issue Oct 11, 2021
The existing lib/isolate/stacktrace_message_test was failing and also
incorrectly written due to "!" in front of the expected stringification
of the stacktrace.

Issue #46623

TEST=vm/dart{,_2}/isolates/fast_object_copy2_test, lib{,_2}/isolate/stacktrace_message_test

Change-Id: I169d8fd7a7c3cb3b8c57e00287565e3a5c622acf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/216041
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>
@WenhaoWu
Copy link

As a small update: This has mostly been adressed (closures, function types, ... can be sent now across SendPorts). Sending StackTrace objects is remaining as is updated documentation.

So has it been scheduled as part of the dart 2.15 release? And if so when do we expect to see dart 2.15?

@mkustermann
Copy link
Member Author

Small update: StackTrace objects can now also be sent to other isolates from the same group - the objects will be shared.

@WenhaoWu It is available in master, beta and will land in stable towards the end of the year. I don't think we can give an exact date (/cc @mit-mit correct me if I'm wrong)

@mit-mit
Copy link
Member

mit-mit commented Oct 13, 2021

Yes, no fixed date, but "towards the end of the year" is a good estimate.

copybara-service bot pushed a commit that referenced this issue Nov 10, 2021
…cise

In addition to making the documentation of [SendPort.send] more precise
by describing what objects are disallowed for sending objects between
isolates within the same isolate group, it also makes [Isolate.exit]
link to [SendPort.send].

Closes #46623

TEST=Updates documentation only.

Change-Id: I9bc1d88faaf2b70589af5e56168976ba89a35558
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/219080
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Alexander Aprelev <[email protected]>
j4qfrost added a commit to conduit-dart/conduit that referenced this issue Mar 14, 2022
j4qfrost added a commit to conduit-dart/conduit that referenced this issue Apr 2, 2022
fix generated test issue

tweak yml

no private

Fix a problem with compiling contorller with List parameter

Update conduit/lib/src/runtime/resource_controller_impl.dart

workflow tweaks

bug fixing test-harness

update yml commands

change closure test to UserTag as per dart-lang/sdk#46623

cache sources

fix port issue

try dev channel

switch around agents

wtf

wtf
j4qfrost added a commit to conduit-dart/conduit that referenced this issue Apr 2, 2022
* refactor: use melos for mono-repo management

* chore(release): publish packages

 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]

* introduce melos, minor fixups, tweak CI

* catch HttpException

* Fix a problem with compiling contorller with List parameter

* Update conduit/lib/src/runtime/resource_controller_impl.dart

* change closure test to UserTag as per dart-lang/sdk#46623

* Fix dart2native compile command (#76)

Fix a problem with compiling contorller with List parameter

Update conduit/lib/src/runtime/resource_controller_impl.dart


change closure test to UserTag as per dart-lang/sdk#46623

* chore(release): publish packages

 - [email protected]
 - [email protected]

Co-authored-by: Slavi Boyanov <[email protected]>
Co-authored-by: Vladi Rachlin <[email protected]>
@adamstyrc
Copy link

I try to send WebSocket object via Isolate.spawn() as a part of List args, but then I get:

Invalid argument(s): Illegal argument in isolate message: (object extends NativeWrapper - Library:'dart:io' Class: _SecureFilter

Any solution to that?

@mkustermann
Copy link
Member Author

I try to send WebSocket object via Isolate.spawn() as a part of List args, but then I get ...

@adamstyrc See my reply here #38964 (comment)

The idea behind isolates is that they are isolated from each other, cannot see side-effects of another isolate and communicate purely via message passing.

Sending native resources such as a WebSocket would create situation where both the sender and receiver have access to it, which is not desirable. An abstraction such as WebSocket may also have as part of it's implementation e.g. registered Timers which wouldn't fire on the receiver isolate.

So it is intentional to disallow this.

What you can do is to make the isolate create network I/O and forward individual messages to the main isolate (if those messages do not contain native resources).

@mraleph
Copy link
Member

mraleph commented May 23, 2022

@mkustermann I wonder if we could consider adding helpers to dart:io which would allow to pass ownership of IO related objects from one isolate to another, e.g. you could imagine something like

// Send socket to another isolate. Native socket gets detached from the 
// Dart object. Calling methods on [socket] will cause [DetachedError] and
// no events will be delivered through [socket] stream.
port.send(detached(socket));

@mkustermann
Copy link
Member Author

@mraleph The thought has crossed my mind before - doing something similar to our existing transferrable bytedata

Here's the issues that made us not look too much into this so far:

  • One needs to handle the case where not all references are wrapped in detached / transferrable (e.g. port.send([socket, detached(socket)]).
  • The Socket itself is mainly a wrapper around other data structures that in return hold on to the native resource. Those wrappers may be in a state that makes it hard to transfer: As mentioned above, sockets may be in a state where they have armed timeout Timers. Such timers would fire in the sender, not the receiver isolate, ...
  • The Socket may have already listeners in the sender isolate, what would happen if it gets detached
  • Right now the Dart VM delegates initialization of core-libraries to the embedder. That means an embedder can decide not to add I/O support to all isolates (e.g. flutter does not add dart:ui support to non-UI isolates).

I think overall our current transitive-copy approach may be in sufficient due to these things.

In the past I have thought that some of these problems could be avoided by letting users control what happens on a send of an object. Objects could e.g. implement abstract class Sendable { Object toSendable(); } which the transitive copy algorithm would invoke, thereby letting user classes decide what happens when an object gets sent (or disallow sending by e.g. throwing an exception). Though this would come with non-trivial performance cost as well as has the issue of invoking user code that may modify parts of the graph that was already handled.

@mraleph Would you suggest baking in dart:io specific knowledge into the VM's SendPort.send() - i.e. special case certain classes?

@mkustermann
Copy link
Member Author

The situation may be easier for Isolate.exit(): Here we could simply transfer all native resource to the receiver isolate, including all open ReceivePorts - effectively merging the entire state of the isolate into another one.

@adamstyrc
Copy link

Thanks @mkustermann for your comprehensive statement! :)

@maks
Copy link

maks commented May 23, 2022

@mkustermann thanks for your detailed response!
One thing I am a bit confused about is how this relates to the shared flag on ServerSocket ?

For me the use-case with websockets is to share incoming connections out to large numbers of (possibly short lived) Isolates. So I'm unclear if that would be possible currently?

Also per the above, I wouldn't find passing Isolates out of exiting Isolates very useful, so I wouldn't be voting for any work to get that implemented.

@aam
Copy link
Contributor

aam commented May 24, 2022

@maks, shared=true on ServerSocket is a special case supported by dart vm runtime for a scenario close to what you are describing: multiple independent dart ServerSocket objects(potentially running in different isolates) all connected to the same OS socket allowing for distribution of incoming requests between concurrently running workers.
There is no dart state sharing happening between those ServerSocket, at a dart level they are not aware of each other.

@maks
Copy link

maks commented May 24, 2022

thanks for the explanation @aam. I guess this might need to go into a separate issue: but is the way the incoming requests are distributed amongst multiple Isolates documented anywhere? In the docs it only says:

The optional argument shared specifies whether additional ServerSocket objects can bind to the same combination of address, port and v6Only. If shared is true and more server sockets from this isolate or other isolates are bound to the port, then the incoming connections will be distributed among all the bound server sockets. Connections can be distributed over multiple isolates this way.

@aam
Copy link
Contributor

aam commented May 25, 2022

Not sure whether documentation is vague on purpose not to pin current implementation down, but currently incoming requests are distributed in round-robin order between all ServerSockets that share one OS socket.

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-isolate
Projects
None yet
Development

No branches or pull requests

10 participants