Skip to content
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

fix: panic in worker when closing at top level #8510

Merged
merged 5 commits into from
Nov 27, 2020

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Nov 26, 2020

Fixes panic occurring in worker when self.close() is called
at the top level, ie. worker shuts down while
module evaluation promise hasn't yet resolved.

Closes #7620
Closes #5334

Supersedes #8395

@bartlomieju bartlomieju changed the title [WIP] fix: panic in worker when closing [WIP] fix: panic in worker when closing at top level Nov 26, 2020
@bartlomieju bartlomieju changed the title [WIP] fix: panic in worker when closing at top level fix: panic in worker when closing at top level Nov 26, 2020
let prefix = "file://";
if (Deno.build.os == "windows") {
prefix += "/";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: I wish had something like Deno.makeTempFile().toUrl()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ry IMO this should be handled by URL. This bit is very problematic in other contexts, especially in dynamic imports: #4641

Unfortunately JS URL can't resolve it properly just like Url Rust crate 😢

// evaluation was complete. This can happen in Web Worker when `self.close()`
// is called at top level.
let result = maybe_result.unwrap_or(Ok(()));
return Poll::Ready(result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really great to have a standalone test in core for this...

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline - too difficult to get test in core.

LGTM

@bartlomieju bartlomieju merged commit 22f951a into denoland:master Nov 27, 2020
@bartlomieju bartlomieju deleted the panic_in_worker branch November 27, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Main thead panics upon worker thread panicing Panic when closing a worker immediately after postMessage
2 participants