Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

Add closed state to Mailbox #26

Merged
merged 2 commits into from
Aug 1, 2024
Merged

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Jul 11, 2024

This is an alternative solution to the same problem described in #25 for sass/dart-sass#2279

On high level we have following logic in dart-sass, and we need a way to reliable stop the loop via mailbox:

Main Isolate:

isolate.kill();
try {
  mailbox.put(Uint8List(0));
} on StateError catch (_) {
  // When delivery of empty message fails due to existing message, the shutdown might lockup.
}

Worker Isolate:

do {
  Uint8List data = mailbox.take();
  if (data.isEmpty) {
    break;
  }
  // do work with data
} while (true);

First, Isolate.kill(priority: Isolate.immediate) won't work when mailbox.take() is blocking. So, instead we call Isolate.kill() first, and then send an empty message to signal the stopping. However, because the sending of empty message can fail, and then depending on the event loop timing, the worker isolate may process the isolate kill event before entering the loop one more time, which works as expected (killed before next iteration of loop); or it may get to the next iteration too quickly before the isolate kill event is processed, and thus cause the VM to lock up because the next mailbox.take() blocks the processing of kill event, causing the isolate to be appear non-responsive.


Adding close method as designed in this PR will allow us to do:

Main Isolate:

isolate.kill();
mailbox.close();

Worker Isolate:

do {
  Uint8List data;
  try {
    data = mailbox.take();
  } on StateError catch (_) {
    break;
  }
  // do work with data
} while (true);

Because mailbox.close() is guaranteed to deliver the signal via StateError on the ongoing or next mailbox.take(), we don't have the problem of failing to send empty message.

@ntkme
Copy link
Contributor Author

ntkme commented Jul 19, 2024

@devoncarew Is this something you can help review, or help identify a reviewer, as it appears @mraleph might not be available.

@devoncarew
Copy link
Contributor

devoncarew commented Jul 19, 2024

@devoncarew Is this something you can help review, or help identify a reviewer, as it appears @mraleph might not be available.

Unfortunately I'm not familiar with this package; @mraleph will be back in a week (@mkustermann - the other likely reviewer - is OOO at the moment as well).

@devoncarew devoncarew requested a review from mraleph July 19, 2024 19:40
lib/mailbox.dart Outdated Show resolved Hide resolved
@ntkme
Copy link
Contributor Author

ntkme commented Aug 1, 2024

@mraleph I've updated the documentation. Can you please take another look when you get a chance?

@mraleph mraleph merged commit bdadfd1 into dart-archive:main Aug 1, 2024
11 checks passed
@ntkme ntkme deleted the closed-state branch August 1, 2024 14:05
devoncarew pushed a commit to dart-lang/labs that referenced this pull request Sep 23, 2024
* Add closed state to Mailbox

* Update documentation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants