-
Notifications
You must be signed in to change notification settings - Fork 174
Fix reporting handled promises as unhandled in tracker #1038
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
Conversation
quickjs.h
Outdated
@@ -1020,10 +1020,9 @@ typedef void JSPromiseHook(JSContext *ctx, JSPromiseHookType type, | |||
JS_EXTERN void JS_SetPromiseHook(JSRuntime *rt, JSPromiseHook promise_hook, | |||
void *opaque); | |||
|
|||
/* is_handled = true means that the rejection is handled */ | |||
/* only gets called on unhandled 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.
@bnoordhuis LMK what you think about this change. I can drop the 2nd commit and leave it more backwards compatible.
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.
Without is_handled
, you can't implement async_hooks, so either it needs to be restored or there should be a promise hook for rejected promises.
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.
Got it, I'll rework it.
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.
Without
is_handled
, you can't implement async_hooks, so either it needs to be restored or there should be a promise hook for rejected promises.
It would be great if the new Promise Hook API could also pick up rejections. :)
It is a bit complicated that the methods for detecting asynchronous events are divided into the PromiseHook API, Promise Rejection Tracker, and FinalizationRegistry, so I think it would be a good idea to integrate the Promise Rejection Tracker into the PromiseHook API. However, I don't know the history of the Promise Rejection Tracker, so if you can't integrate it, that's fine.
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.
I suppose I could add a new hook JS_PROMISE_HOOK_REJECT
and then an extra API JS_PromiseIsHandled
and we could drop this altogether.
Thoughts @bnoordhuis ?
quickjs.h
Outdated
@@ -1020,10 +1020,9 @@ typedef void JSPromiseHook(JSContext *ctx, JSPromiseHookType type, | |||
JS_EXTERN void JS_SetPromiseHook(JSRuntime *rt, JSPromiseHook promise_hook, | |||
void *opaque); | |||
|
|||
/* is_handled = true means that the rejection is handled */ | |||
/* only gets called on unhandled 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.
Without is_handled
, you can't implement async_hooks, so either it needs to be restored or there should be a promise hook for rejected promises.
77bb476
to
0ee636b
Compare
Enqueue a job which will perform the check after all reactions. Fixes: #39
0ee636b
to
2358d21
Compare
@bnoordhuis Updated, PTAL! |
@saghul Still reproducible on this: const asyncFunction = async () => {
throw new Error('this function rejects')
}
async function run() {
const error = await Promise.resolve().then(function () {
var resultPromise = asyncFunction();
return Promise.resolve().then(function () {
return resultPromise;
}).then(function () {
return null;
}).catch(function (e) {
return e;
});
});
console.log('Got this error:', error)
}
run().then(() => console.log('done')) Code mostly extracted from http://npmjs.com/package/assert |
Dammit, I'll take another look! Thanks for the test case! |
Simplified test case: const error = await Promise.resolve().then(() => Promise.reject('reject')).catch(e => e)
console.log('Got this error:', error) |
Still has a bug, but mostly works Refs: quickjs-ng/quickjs#1038 (comment)
I don't believe this patch is correct—it introduces behavior that's inconsistent with Web browser implementations. Take the following code example in a web browser. Only window.addEventListener('unhandledrejection', event => {
console.log('unhandledrejection fired: ' + event.reason);
});
window.addEventListener('rejectionhandled', event => {
console.log('rejectionhandled fired: ' + event.reason);
});
function generateRejectedPromise(isEventuallyHandled) {
// Create a promise which immediately rejects with a given reason.
var rejectedPromise = Promise.reject('Error at ' + new Date().toLocaleTimeString());
rejectedPromise.catch(() => {
console.log('rejected');
});
}
generateRejectedPromise(true); This demonstrates that the caller (the For users who register a callback with Reference: However, the current patch sets |
I don't believe the Promise feature in Bellard's 2021-03-27 version had any issues. The problems stemmed from a misunderstanding of how Promise rejections were handled and reported. I've used this implementation in WebF for over four years, powering frameworks like React, Vue, and Jasmine, as well as open-source tools such as Ant Design—from its initial open-source phase to its application in enterprise crypto projects. Rolling back to Bellard’s original implementation and referring to the WebF usage should resolve the issues. |
I don't think the intention was that from the start though. Rather, how to terminate the process in unhandled rejection cases which server runtimes implement.
How would a server runtime know when to terminate the process in case of an unhandled rejection then?
Note I made a follow-up PR handling more corner cases. |
I think the termination of the process should depend on the currently enqueued microtasks or any active event listeners. Regardless of whether the callback executes successfully or throws an exception, the process should not terminate immediately. If an exception is thrown, it should simply emit an event on globalThis. |
Node, Deno and Bun disagree so there is ample precedent here. |
It sounds like you're thinking of what Node.js does but that's in no way standard. There is no standard, it's up to the embedder - not the JS engine - to decide what's best. |
Emm, if you wish to terminate the process when an unhandled async exception is thrown, it might be better to perform the check at the end of this enqueue job instead of immediately. This is because the user might catch and handle the error after the promise is created. // Finish all the pending jobs
int finished = JS_ExecutePendingJob(script_state_.runtime(), &pctx);
while (finished != 0) {
finished = JS_ExecutePendingJob(script_state_.runtime(), &pctx);
if (finished == -1) {
break;
}
}
// Terminate the process if there are unhandled exceptions.
if (haveUnhandledExceptions) {
exit(1);
} |
Let me give that a try. |
This reverts a75498b and a4a5a17 (except the tests) and adapts bellard/quickjs@3c39307 which also passes the tests and feels like a better solution overall. Ref: #1038
Fix: #1058 |
This reverts a75498b and a4a5a17 (except the tests) and adapts bellard/quickjs@3c39307 which also passes the tests and feels like a better solution overall. Ref: #1038
This reverts a75498b and a4a5a17 (except the tests) and adapts bellard/quickjs@3c39307 which also passes the tests and feels like a better solution overall. Ref: #1038
Enqueue a job which will perform the check after all reactions. Also drop the case in which the tracker gets called with
true
, the handler now only gets called when the promise rejection is unhandled.Fixes: #39