Unhandled promise tracker called incorrectly

[saghul]

See: https://github.com/saghul/txiki.js/issues/310 and https://github.com/bellard/quickjs/issues/112

[andrieshiemstra]

We also had this issue and some discussion about it (and a workaround)... I'm still not sure if it should be considered a bug... more of an nuisance... see https://github.com/HiRoFa/quickjs_es_runtime/issues/74

some googling leads me to believe browsers just update the console error after .catch is actually called (https://github.com/nodejs/help/issues/4286) which we could also implement (put unhandled promise in a separate list and only trigger unhandledTracker in runPendingJobs if no more pending jobs and promise still unhandled?) but even then... async is async.. so you could solve this..

new Promise((res, rej) => {
    rej("sync rejection")
}).catch((e) => {
    console.log("handled");
})

But not this even if the problem is the same, catch is called later...

const prom = new Promise((res, rej) => {
    rej("sync rejection")
});
setTimeout(() => {
prom.catch((e) => {
    console.log("handled");
})
}, 2000);

(e.g. firefox also fixes the first but not the latter...)

[saghul]

Aha, TIL!

I think one of the key things here is the handler is called more than once. First with the promise not being handled (is_handled would be 0) then with 1. Maybe that's ok, since users wouldn't want to do anything in that case?

[andrieshiemstra]

how about this:

in quickjs.c static void fulfill_or_reject_promise is the only method which checks if a promise is handled

if (s->promise_state == JS_PROMISE_REJECTED && !s->is_handled) {
        JSRuntime *rt = ctx->rt;
        if (rt->host_promise_rejection_tracker) {
            rt->host_promise_rejection_tracker(ctx, promise, value, FALSE,
                                               rt->host_promise_rejection_tracker_opaque);
        }
    }
}

Instead of calling the rejection tracker directly we increase the refcount of the promise and enqueue a job which does something like this

if (s->promise_state == JS_PROMISE_REJECTED && !s->is_handled) {
        JSRuntime *rt = ctx->rt;
        if (rt->host_promise_rejection_tracker) {
            rt->host_promise_rejection_tracker(ctx, promise, value, FALSE,
                                               rt->host_promise_rejection_tracker_opaque);
        }
    }
    JS_Free(promise)

that way the is_handled check occurs as a new tick in the eventloop and after .catch is called. and if the promise is unhandled then the tracker is called just once..

Promise fullfillment is unchanged, and the only change is that the rejection call is done in a later tick (and in that case a refcount is upped and downed once extra... and only IF there is a rejectiontracker..

[saghul]

The tracker is also called in perform_promise_then though.

There is already a promise_reaction_job that runs after promises. Perhaps that can be augmented?

[andrieshiemstra]

The tracker is also called in perform_promise_then though.

hmm yeah, but could have same change as above..

There is already a promise_reaction_job that runs after promises. Perhaps that can be augmented?

if I'm reading the code correctly the promise_reaction_job is enqueued for every promise reaction list_for_each_safe(el, el1, &s->promise_reactions[is_reject]) {? so it would not be added if i create a promise without thens or catches right? so this would not work..

new Promise(() => {throw Error("i will not trigger the promise rejection handler")});

[saghul]

Hum, looks like it indeed.