Fix reporting handled promises as unhandled in tracker #1038
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.
@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.
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.
Got it, I'll rework it.
There was a problem hiding this comment.
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.
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.
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.
saghul
force-pushed
the
fix-unhandled-rejection-tracker
branch
2 times, most recently
from
77bb476 to
0ee636b
Compare
Enqueue a job which will perform the check after all reactions. Fixes: #39
saghul
force-pushed
the
fix-unhandled-rejection-tracker
branch
from
0ee636b to
2358d21
Compare
|
@bnoordhuis Updated, PTAL! |
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