Unhandled promise tracker called incorrectly

[saghul]

See: saghul/txiki.js#310 and bellard/quickjs#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 HiRoFa/quickjs_es_runtime#74

some googling leads me to believe browsers just update the console error after .catch is actually called (nodejs/help#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.

[richarddd]

I've started investigating this. The problem is that the results of promises that throw error or is immediately resolved (aka Promise.resolve) does this synchronously rather than doing so in a pending job. So QuickJS doesn't know at that time weather a promise has attached callbacks (then,catch etc) or not. The solution to this problem would be to rebuild the Promise implementation so that the result of promises are always checked in a pending job. For example:

const p = new Promise(() => {
   console.log(1)
   throw new Error("kaboom");
})
console.log(2)

Should report unhandled promise rejection AFTER logging 2:

qjs --unhandled-rejection promise.js
1
Possibly unhandled promise rejection: Error: kaboom
    at <anonymous> (promise.js:3)
    at Promise (native)
    at <eval> (promise.js:4)

2

So basically we should:

1. Execute promise constuctor
2. If constructor errors, or resolves, store result in promise state
3. Queue pending job to check promise result.
4. When pending job then is executed, check if we have catch handler. If no catch handler throw Uncaught (in promise) / unhandled promise rejection error.

This is different from the current implementation where we only queue pending job when callbacks are attached:
https://github.com/quickjs-ng/quickjs/blob/966dbfc1f96a57d2cdeec89474dd540e9c87e240/quickjs.c#L48381

@saghul @bnoordhuis @chqrlie thoughts?

[bnoordhuis]

Calling the reject function synchronously from the promise constructor (but still return the promise object) seems to be per spec; V8 does that, too.¹

Deferring the fulfill_or_reject_promise call that js_promise_constructor currently calls synchronously seems reasonable but the challenge is to only defer in that specific scenario.

The function you're looking for is js_promise_resolve_function_call² but it can be called either synchronously or asynchronously. An already asynchronous call should not defer.

---

¹ Not sure if it's actually observable though.

² Contrary to what its name suggests, it also handles rejections.

[richarddd]

challenge is to only defer in that specific scenario

Not quite sure I follow. When should we not defer?

[saghul]

Not sure I agree with the initial premise, let's explore!

The Promise creation spec: https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-promise-executor

It says that if calling the executor is an abrupt termiantion we should call reject(e).

Now let's look at how that reject function is created: https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-createresolvingfunctions

Let's now look at step 7, so the reject function steps: https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-createresolvingfunctions

Then finally the RejectPromise oepration: https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-rejectpromise

While V8 seems to enqueue the call to the handler, I don't see any mention to it here.

Is my reading correct or did I miss anything?

Note that "but everyone does it" is probably a valid answer here! :-)

[richarddd]

Note that "but everyone does it" is probably a valid answer here! :-)

Yeah probably. I don't think the spec specifies if it should be enqueue or not. I assume this is an effect of it being impossible to detect if handlers/callbacks are attached otherwise since the constructor always runs first

[richarddd]

@saghul @bnoordhuis I've got an idea. What if we schedule a job from promise constructor whose sole purpose is to check if handlers are attached if constructor returns an exception?
Suggestion:

1. Don't return exception from promise constructor to caller
2. If constructor throws, store that exception (with stacktrace) in the promise states data and queue a pending_job check_promise_handlers
3. If state->handled is false inside check_promise_handlers, we warn about unhandled promise rejections.

I suspect that will work without a major promise implementation refactor.

[bnoordhuis]

If test262 is happy, I suppose I'm happy, but it sounds like that would be an observable change?

You can test that if you add a JS_SetHostPromiseRejectionTracker call to run-test262.c, then recompile and run run-test262 -c test262.conf