Implement Isloate.spawnSync

[bsutton]

I'm the author of DCli which utilises waitFor to synchronously spawn processes.

With the deprecation of waitFor I'm currently stuck without a way to maintain the current api.

This issue tracking the deprecation of waitFor is here:

#52121

The core issue is that I need to spawn an isolate synchronously.

I'm not looking for the isolate to run to completion, rather just the ability to start the isolate synchronously.

With the current api

final isolate = Isolate.spawn<Args>(_runProcess, Args(commandToRun, mailbox));

/// The following code creates a dead lock as the isolate hasn't started and so doesn't
/// put anything into the mailbox.
mailbox.take(); // sync call that doesn't return util the isolate starts and puts a message into the mailbox

final isolate = Isolate.spawnSync<Args>(_runProcess, Args(commandToRun, mailbox));
/// I'm now guaranteed the isolate is running which means it will eventually place something in the mailbox.
mailbox.take(); // sync call.

I've tried an number of paths to get around the spawn issue but it appears that if you make a sync call (e.g. mailbox.take) immediately after calling Isolate.spawn then the spawned isolate doesn't start. I assume the current spawn logic is relying on a async call to be given processing time to start.

Unless I'm wrong about how spawn works, I've hit a brick wall in migrating dcli off of waitfor. I have resolutions for all of the other code except this rather critical piece.

@mraleph
@Hixie
@itsjustkevin

I need this enhancement before waitFor is removed from the dart api.

[tsavo-at-pieces]

Good looks! +1

[nex3]

I agree that this is likely to be necessary for moving off all uses of waitFor().

[bsutton]

I've would like to see waitfor un-deproacated until this is in the stable release. I have everything else ready but dcli is broken against stable until we have something like this as a solution.

…

On Wed, 3 Apr 2024, 11:17 am Natalie Weizenbaum, ***@***.***> wrote: I agree that this is likely to be necessary for moving off *all* uses of waitFor(). — Reply to this email directly, view it on GitHub <#55356 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OFH2MFZW5OSSIOCQNDY3NDCVAVCNFSM6AAAAABFUH6LVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGMYTKMJYGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[lrhn]

I assume the current spawn logic is relying on a async call to be given processing time to start.

That should not be necessary.
At the time the future is returned, the spawn should be underway, with no more local-isolate work necessary. The local future won't be completed while the local isolate is blocked, but if you don't care about that, it shouldn't be a problem.

If it doesn't work that way, I'll consider it a bug.

Could it be a problem that you're receiving events on other ports, which cannot be processed, and that somehow blocks the event to the port you're using for the mailbox? If it's using ports, I haven't checked the implemention. (If so, that should probably also be considered a bug, if nowhere else then in the mailbox code.)

@mraleph

[lrhn]

Actually, the problem is probably that the spawn fails, which is reported as an asynchronous exception in the returned future. Which means it's not reported when the isolate is blocked, but since the isolate never started, the mailbox won't get any messages to unlock it.
That's a deadlock.

A reason it could throw might be that you're sending the mailbox object directly, and it's not sendable.
Change the code to:

final isolate = Isolate.spawn<Args>(_runProcess, Args(commandToRun, mailbox.asSendable));

and unwrap the Sendable<MailBox> using .materialize() at the other end.
If I do that, my similar code runs even when mailbox.take() is right after Isolate.spawn.

Or, what I would do, make the Args constructor call .asSendable on its argument and store it as a Sendable<MailBox>. The give it a mailbox getter which re-materializes the mailbox if needed. If you want to be absolutely certain that the mailbox isn't stored on the object, in case you want to send it to another isolate, use an Expando.

Something like:

class Args {
  static Expando<Maibox> _mailboxCache = Expando();
  final Sendable<Mailbox> _mailboxPackage;
  final Command commandToRun;
  Args(this.commandToRun, MailBox mailbox) : _mailboxPackage = mailbox.asSendable {
    _mailboxCache[this] = mailbox;
  }
  Mailbox get mailbox => _mailboxCache ??= _mailboxPackage.materialize();
}

That should contain the sending logic, while still letting both ends use the object like today.

It is a problem that isolate spawn can fail later, but that's what asynchronous computations do.

A synchronous spawn would prevent that. Alternatively, what is needed is not a synchronous spawn, but just synchronous errors. If a flag to Isolate.spawn could force synchronous validation of the sendability of the function and argument (which I bet already happen), and then to throw a validation error synchronously, then the actual spawning (which should then not fail for any reason other than out-of-memory) could still be asynchronous.
That's untraditional (usually an function returning a future will report its errors in that future), but if you ask for it (Isolate.spawn(..., eagerError: true)), then it should be OK.

Or maybe mailbox.take needs a timeout. If no message has been received in time, it throws a TimeoutException instead, which allows reporting an error instead of dead-locking. Tricky, if we're doing arbitrary user requested operations. Maybe we can have the operations send progress reports in the mailbox, like:

var isolateFuture = Isolate.spawn(_run, _Args(data, mailbox));
try {
  Uint8List msg;
  do {
    msg = mailbox.take(timeout: const Duration(seconds: 5);
  } while (_isStillWorking(msg)); // Standard IAmAlive report sent every second or so, not final result.
  return _parseResult(msg);
} on TimeoutException {
  isolateFuture.then((isolate) {
    isolate.kill();
  });
  throw OperationFailedException("User operation stalled");
}

Then you could build a protocol on top of the mailbox, instead of only using it for the final result.
(But without a time-out, it's limited how that can be used to prevent deadlocks.)

[tsavo-at-pieces]

@lrhn I was thinking the same and asked that as well in the other issue thread: #52121 (comment)

@bsutton are you sure this is not the case?

[tsavo-at-pieces]

Just kidding - this was the code that I updated to use native_synchronization

Specifically:

SendPort _connectSendPort(ProcessChannel channel) {
  /// take the initial message which contains
  /// the channels sendPort id.
  final msg = channel.send.take();

  return NativeCalls.connectToPort(msg);
}

from this file

---

is referencing:

class ProcessChannel {
  ProcessChannel()
      : pipeMode = false,
        response = Mailbox(),
        send = Mailbox();

in this file

---

And I forgot that I had updated this strong type to verify here:

void _startIsolate(ProcessSettings processSettings, ProcessChannel channel) {
unawaited(Isolate.spawn<List<Sendable<Mailbox>>>((mailboxes) async {

at this line

---

where the Sendables are passed in as so:

      List<Sendable<Mailbox>>.from([
        channel.send.asSendable,
        channel.response.asSendable,
      ]),
      debugName: 'ProcessInIsolate'));

at this line

@lrhn Maybe you could triple spot check me here just to make sure I didn't overlook something obvious?

[bsutton]

Given the above feedback i decided to write a few simplified experiments.

The following works as expected:

void main() {
  unawaited(Isolate.spawn(_run, 'hellow'));
  sleep(const Duration(seconds: 10));
}

void _run(String arg) {
  print('Hello from isolate: $arg');
}

This seems to support @lrhn statement that the spawn should proceed even if a sync call is made immediately after the spawn occurs.

[bsutton]

Doing a simple mailbox.take also allows the isolate to spawn without issue:

void test2() {
  final mailbox = Mailbox();
  unawaited(Isolate.spawn(_run, 'hellow'));
  mailbox.take();
}

void _run(String arg) {
  print('Hello from isolate: $arg');
}

[lrhn]

@lrhn Maybe you could triple spot check me here just to make sure I didn't overlook something obvious?

Not going to try to grok all the code, and don't know enough about the native synchronization functions, so I'll just give the completely generic recommendation to avoid the risk of over-capturing with closures.

The (mailboxes) async { ... } closure captures the processSetting variable from the surrounding scope, and is passed a List<Sendable<Mailbox>> as argument. The channel containing the original unsendable Mailboxes is also in scope. If the closure decides to (over-)capture that variable too, it will be unsendable.
It probably won't, because there is no closure capturing channel at all. It's still something that can go wrong if the compiler makes a wrong deduction somewhere.

I'd still consider moving the closure to be its own static function, and pass the processSettings as an argument too, using a record for the argument and patterns to destructure, so something like:

void _runInIsolate((ProcessSettings, Sendable<MailBox>, Sendable<Mailbox>) args) {
  final (processSettings, sendableSendBox, sendableResponseBox) = args;
  final sendBox = sendableSendBox.materialize();
  final responseBox = sendableResponseBox.materialize();
  // ...
}

Or pack up the mailboxes before calling _startIsolate(ProcessSettings processSettings, List<Sendable<Mailbox>> mailboxes) {... } so the unsendable mailboxes are not in the scope at all.

Whether the following logic is correct, that's for tests to decide 😁.

[bsutton]

I've made progress so thanks for the input. I will update in a couple of days once I get further down the path.

…

On Thu, 4 Apr 2024, 6:56 pm Lasse R.H. Nielsen, ***@***.***> wrote: @lrhn <https://github.com/lrhn> Maybe you could triple spot check me here just to make sure I didn't overlook something obvious? Not going to try to grok all the code, and don't know enough about the native synchronization functions, so I'll just give the completely generic recommendation to avoid the risk of over-capturing with closures. The (mailboxes) async { ... } closure captures the processSetting variable from the surrounding scope, and is passed a List<Sendable<Mailbox>> as argument. The channel containing the original unsendable Mailboxes is also in scope. If the closure decides to (over-)capture that variable too, it will be unsendable. It *probably* won't, because there is no closure capturing channel at all. It's still something that can go wrong if the compiler makes a wrong deduction somewhere. I'd still consider moving the closure to be its own static function, and pass the processSettings as an argument too, using a record for the argument and patterns to destructure, so something like: void _runInIsolate((ProcessSettings, Sendable<MailBox>, Sendable<Mailbox>) args) { final (processSettings, sendableSendBox, sendableResponseBox) = args; final sendBox = sendableSendBox.materialize(); final responseBox = sendableResponseBox.materialize(); // ... } Or pack up the mailboxes before calling _startIsolate(ProcessSettings processSettings, List<Sendable<Mailbox>> mailboxes) {... } so the unsendable mailboxes are not in the scope at all. Whether the following logic is correct, that's for tests to decide 😁. — Reply to this email directly, view it on GitHub <#55356 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OHUIHQJU6Z664P7PYDY3UBR5AVCNFSM6AAAAABFUH6LVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZWGQ2DOMJVGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>