fix(workers): Don't panic when a worker's parent thread stops running #12156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
As it turns out, terminating the worker when
|
This panic could happen in the following cases: - A non-fatal error being thrown from a worker, that doesn't terminate the worker's execution, but propagates to the main thread without being handled, and makes the main thread terminate. - A nested worker being alive while its parent worker gets terminated. - A race condition if the main event loop terminates the worker as part of its last task, but the worker doesn't fully terminate before the main event loop stops running. This panic happens because a worker's event loop should have pending ops as long as the worker isn't closed or terminated – but if an event loop finishes running while it has living workers, its associated `WorkerThread` structs will be dropped, closing the channels that keep those ops pending. This change adds a `Drop` implementation to `WorkerThread`, which terminates the worker without waiting for a response. This fixes the panic, and makes it so nested workers are automatically terminated once any of their ancestors is closed or terminated. This change also refactors a worker's termination code into a `WorkerThread::terminate()` method. Closes denoland#11342.
andreubotella
force-pushed
the
worker-drop-handle-race
branch
from
6cd79db to
e20dd13
Compare
andreubotella
changed the title
WebWorkerHandle should terminate the worker
bartlomieju
approved these changes
Sep 22, 2021
Comment on lines
+87
to
+88
| .expect("Worker thread panicked") | ||
| .expect("Panic in worker event loop"); |
There was a problem hiding this comment.
I'm not sure if we should be expect()ing here. This would cause another panic if something goes wrong in worker and might be prone to other race conditions.
There was a problem hiding this comment.
Although that doesn't change current behavior so I guess it's fine for now
andreubotella
pushed a commit
to andreubotella/deno
that referenced
this pull request
Sep 24, 2021
Before denoland#12156, closing a worker which had children would cause a panic (denoland#11342 (comment)). After that PR, closing a worker will also close any child workers.
piscisaureus
pushed a commit
that referenced
this pull request
Sep 25, 2021
…12215) Before #12156, closing a worker which had children would cause a panic (#11342 (comment)). After that PR, closing a worker will also close any child workers.
caspervonb
pushed a commit
to caspervonb/deno
that referenced
this pull request
Sep 26, 2021
…denoland#12156) This panic could happen in the following cases: - A non-fatal error being thrown from a worker, that doesn't terminate the worker's execution, but propagates to the main thread without being handled, and makes the main thread terminate. - A nested worker being alive while its parent worker gets terminated. - A race condition if the main event loop terminates the worker as part of its last task, but the worker doesn't fully terminate before the main event loop stops running. This panic happens because a worker's event loop should have pending ops as long as the worker isn't closed or terminated – but if an event loop finishes running while it has living workers, its associated `WorkerThread` structs will be dropped, closing the channels that keep those ops pending. This change adds a `Drop` implementation to `WorkerThread`, which terminates the worker without waiting for a response. This fixes the panic, and makes it so nested workers are automatically terminated once any of their ancestors is closed or terminated. This change also refactors a worker's termination code into a `WorkerThread::terminate()` method. Closes denoland#11342. Co-authored-by: Bartek Iwańczuk <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This panic could happen in the following cases:
This panic happens because a worker's event loop should have pending ops as long as the worker isn't closed or terminated – but if an event loop finishes running while it has living workers, its associated
WorkerThreadstructs will be dropped, closing the channels that keep those ops pending.This change adds a
Dropimplementation toWorkerThread, which terminates the worker without waiting for a response. This fixes the panic, and makes it so nested workers are automatically terminated once any of their ancestors is closed or terminated.This change also refactors a worker's termination code into a
WorkerThread::terminate()method.Closes #11342.