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(core) assertion failure in case of js error #9721

Merged
merged 3 commits into from
Mar 8, 2021
Merged

fix(core) assertion failure in case of js error #9721

merged 3 commits into from
Mar 8, 2021

Conversation

inteon
Copy link
Contributor

@inteon inteon commented Mar 7, 2021

Extracted this bugfix from #9457.

In case JavaScript throws an unhandled error, part of the "shared_queue" could be still unprocessed. If this is the case; throw the js runtime error instead of asserting on the queue size not being 0.

@bartlomieju
Copy link
Member

@inteon could you provide a test for this edge case?

@inteon
Copy link
Contributor Author

inteon commented Mar 8, 2021

@bartlomieju done

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Nice fix! Thank you @inteon

@@ -2024,6 +2027,49 @@ pub mod tests {
});
}

#[test]
fn shared_queue_not_empty_when_js_error() {
Copy link
Member

Choose a reason for hiding this comment

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

I've verified that this test fails on main:

---- runtime::tests::shared_queue_not_empty_when_js_error stdout ----
thread 'runtime::tests::shared_queue_not_empty_when_js_error' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`', core/runtime.rs:1438:7
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@bartlomieju bartlomieju merged commit 4cd1f3d into denoland:main Mar 8, 2021
@inteon inteon deleted the move_minimal_ops_to_core_step3 branch March 8, 2021 14:54
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.

2 participants