-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
perf(ext/ffi): switch from middleware to tasks #21239
Conversation
bb3f951
to
a922941
Compare
2d0c89e
to
a47ce60
Compare
9ab4a7d
to
fe543d3
Compare
e5452ed
to
5589e77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just curious about how you did error handling.
ext/ffi/callback.rs
Outdated
}; | ||
|
||
async_work_sender.spawn_blocking(move |scope| { | ||
args.run(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What happens with errors thrown from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because these are tasks and we're technically violating the contract by allowing them to bubble into the scope, they end up as "uncaught errors". It doesn't kill the runtime but it isn't a great situation.
It should not be the same as our current state of affairs, however, but I suppose we could add a tc_scope
here and print a last-ditch message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we will need to figure out what to do with this test as it's failing right now -- either we could punt with a TODO for later, or try to add the last-ditch handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I did add the error and unhandled rejection listeners in the test but they were not getting fired. Are you sure the errors end up as properly handled uncaught errors/rejections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me dig into this a bit and see what is going on before we land it...
); | ||
|
||
globalThis.addEventListener("error", (data) => { | ||
console.log("Unhandled error"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never gets logged
console.log("Unhandled error"); | ||
data.preventDefault(); | ||
}); | ||
globalThis.onerror = (data) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nor does this get called; this also should be redundant with the one above.
Deno-side changes for denoland/deno_core#350