Skip to content

When reporting illegal object SendPort.send should print retaining path and hide implementation internal classes #51115

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
dnfield opened this issue Jan 24, 2023 · 9 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-isolate P2 A bug or feature request we're likely to work on

Comments

@dnfield
Copy link
Contributor

dnfield commented Jan 24, 2023

See e.g. simolus3/drift#2279 (comment)

 dart test test/isolate_test.dart
00:01 +8 -1: in same isolate with explicit serialization can run deletes, updates and batches [E]                                                                                                                                                                                                                    
  Invalid argument(s): Illegal argument in isolate message: (object is a SuspendState)
  dart:isolate                   Isolate.run
  test/isolate_test.dart 255:19  _runTests.<fn>

I don't know where SuspendState is defined. It's not defined in Drift AFAICT, and I don't see it when I search on api.dart.dev.

So basically, I don't really know what's wrong or where to start with fixing this issue.

@dnfield dnfield added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-isolate labels Jan 24, 2023
@alexmarkov
Copy link
Contributor

This error likely indicates that you're trying to send a Future returned from a suspended async function to another isolate.
We should detect this case explicitly and throw an error with a more detailed explanation message.

@simolus3
Copy link
Contributor

simolus3 commented Jan 24, 2023

Possibly also related to #48592.

@a-siva
Copy link
Contributor

a-siva commented Jan 24, 2023

//cc @aam

@dnfield
Copy link
Contributor Author

dnfield commented Jan 24, 2023

This error likely indicates that you're trying to send a Future returned from a suspended async function to another isolate. We should detect this case explicitly and throw an error with a more detailed explanation message.

Can you explain a bit more about why that doesn't work and what should be done instead?

@alexmarkov
Copy link
Contributor

Future represents a result of asynchronous computation which is happening within a certain isolate. An ability to await for the completion of that computation and get its result from another isolate would bypass message passing and violate isolation. Sending non-constant objects between isolates involve copying them, so even if we could send futures, the receiving isolate would get its own copy of a Future, which will not be completed by the original asynchronous computation and will not get its result.

Internally, SuspendState object represents a stack frame for a suspended asynchronous computation. It is not useful to make a copy of SuspendState in another isolate, as it can only be resumed within the original isolate.

@mraleph mraleph changed the title Isolate.run errors are opaque/difficult to debug When reporting illegal object SendPort.send should print retaining path Jan 26, 2023
@mraleph
Copy link
Member

mraleph commented Jan 26, 2023

I think there are two parts here:

  • When reporting illegal object in the message SendPort.send should print retaining path to making finding illegal object simpler
  • When reporting some implementation internal object (like SuspendState) we should abort sending earlier and report a clear error message

/cc @aam @mkustermann

I think we are definetely overdue to fix this error message. I even thought we might have an issue already tracking this work - but I could not find it.

Would it be something you could look at @aam?

PS. I also think that there is an interesting question around sending Stream and Future instances to another isolate. I have previously suggested that sending a Future through SendPort should establish a channel between two isolates, e.g. if the code awaits the Future on the receiving side then the Future will be awaited on the sending side as well and completion should be forwarded through the port. Similar with Stream. Though @lrhn suggested that doing this automatically is a bit too much magic - and maybe instead we should just have utility classes which do this.

@mraleph mraleph changed the title When reporting illegal object SendPort.send should print retaining path When reporting illegal object SendPort.send should print retaining path and hide implementation internal classes Jan 26, 2023
@mkustermann
Copy link
Member

I even thought we might have an issue already tracking this work - but I could not find it.

Maybe you were looking for #48592?

@lrhn
Copy link
Member

lrhn commented Jan 26, 2023

Creating an automatic channel between two isolates when sending a stream or future is too-much-magic mainly because it requires an open receive port in one or both directions. These ports are implicit so users have no way to close them, and if they did, no way to know when to close them.

If the VM can do something magic under the hood, where a _Future serialized to another isolate would create implicit connections that won't keep any isolate alive, and which know when the other end stops being relevant, then that'd probably be safe. But if we can do that, there's so many other things I'd like to use such connections for, to the point where I'd deliberately send futures and streams around to create such connections :)

@aam aam self-assigned this Jan 26, 2023
@alexmarkov
Copy link
Contributor

Automatically synchronizing Futures/Streams between isolates makes isolates less isolated. Maybe this should be done through a separate new API, only for a new kind of Future/Stream (SharedFuture, SharedStream etc) so it would only happen if user explicitly asked for such synchronization.

Also, if we're willing to breach isolation and open more communication channels between isolates, then we should also consider creating collections which are automatically updated when modified from another isolate (SharedList, SharedMap, SharedSet etc).

@a-siva a-siva added the P2 A bug or feature request we're likely to work on label Jan 30, 2023
copybara-service bot pushed a commit that referenced this issue Feb 23, 2023
…ception.

The exception contains retaining path that looks like this:
```
Invalid argument(s): Illegal argument in isolate message: (object extends NativeWrapper - Library:'dart:io' Class: _RandomAccessFileOpsImpl@14069316)
 <- Library:'dart:io' Class: _RandomAccessFile@14069316
 <- Library:'file:///vm/dart/isolates/send_unsupported_objects_test.dart' Class: SomeLog
 <- Library:'file:///vm/dart/isolates/send_unsupported_objects_test.dart' Class: SomeState
 <-  Class: Context
 <- Library:'dart:core' Class: _Closure@0150898

```

BUG=#51115
BUG=#48592
TEST=send_unsupported_objects_test

Change-Id: I022e693adccf43a7d2c95e1c7283fd7f210cf1d7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280523
Commit-Queue: Alexander Aprelev <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>
@aam aam closed this as completed Apr 10, 2023
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 P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

8 participants