Skip to content
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

Add PromiseRejectionHandler #2319

Closed
wants to merge 1 commit into from
Closed

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Jan 6, 2022

This just logs every time a promise gets rejected but has not reject
handler (yet).

The message is basically the one firefox gives.

The simplest variant of #2318

This just logs every time a promise gets rejected but has not reject
handler (yet).

The message is basically the one firefox gives.

The simplest variant of #2318
@github-actions github-actions bot requested review from codebien and yorugac January 6, 2022 14:39
@@ -307,6 +307,28 @@ func (b *Bundle) instantiate(logger logrus.FieldLogger, rt *goja.Runtime, init *
}
rt.Set("__ENV", env)
rt.Set("__VU", vuID)
rt.SetPromiseRejectionTracker(func(p *goja.Promise, op goja.PromiseRejectionOperation) {
// TODO this only handles the case where a promise get's rejected without having a reject handler added.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO this only handles the case where a promise get's rejected without having a reject handler added.
// TODO this only handles the case where a promise gets rejected without having a reject handler added.

Also, something doesn't seem grammatically correct in the bottom 2 lines, but I don't understand exactly what you're trying to say, so I can't offer a correction 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from https://tc39.es/ecma262/#sec-host-promise-rejection-tracker 's Note 1

Note 1

HostPromiseRejectionTracker is called in two scenarios:

  • When a promise is rejected without any handlers, it is called with its operation argument set to "reject".
  • When a handler is added to a rejected promise for the first time, it is called with its operation argument set to "handle".

We handle only the first one

the rest of the note is

A typical implementation of HostPromiseRejectionTracker might try to notify developers of unhandled rejections, while also being careful to notify them if such previous notifications are later invalidated by new handlers being attached.

this only implements up to the the , as ... IMO and IME implementations actually just don't notify you if a promise gets rejected without a handler but then later gets handlers ... for some definition of "later".

var p = somethingReturningAPromise()
// here it has *no* handlers 
// if here it happens to either fulfill or get rejected nothing happens, including if 
// `somethingReturningAPromise` gets an exception  
// if it's rejected though SetPromiseRejectionTracker will be called with `op` equal to "reject"

p.then((a)=>console.log("succes", a)
//^ will add a handler for when the promise is fulfilled *and* when it becomes 
// "fulfilled" it will queue the argument (the arrow function)
// 
// it will *also* queue it if it has already fulfilled,
// even if it has fulfiled before and that already queued something.
// so if we repeat this line *and* the promise is already fulfilled 
// it will get it's argument to `then` queued twice for example

p.catch((bad)=> console.log("promise go rejected")) 
// ^ this will now both call SetPromiseRejectionTracker with "handled"
// and queue the argument to catch *if* the promise was already rejected.
// the same applies as for `.then` if called multiple times and the promise
//  is already rejected it will queue multiple times *by specification* 

so for code such as

var p = promiseSomething();
// here p gets rejected somehow :shrug:
// SetPromiseRejectionTracker gets fired and we get a log
p.then((something)=>console.log("cool")
// ^ doesn't queue the body as the promise *is rejected*
// if this is the end, it's really hard to find what goes on as you
//  just made a promise set "then" and if we don't have 
// SetPromiseRejectionTracker, nothing would've even been printed

// if you add
p.catch((b)=>console.log(b)); 
// this will now call SetPromiseRejectionTracker with `op` 
// equal to "handled" and actually queue the provided (arrow) function to be executed

With our current code it will execute this code, logging that unhandled exception (in promise) was caught and then when it finished (b)=>console.log(b) will be executed, where b is the value of whatever was the promise rejected with (possibly an exception). If there were multiple calls for catch and they were made after the promise was rejected all of them will be queued.

the really problematic variants are stuff like the ones in #779 (comment) explained in #779 (comment)

const timeout = (ms) => { // this technically is async but it doesn't really matter
  return new Promise((resolve, reject) => {
     // don't ask me why the function is resolve, but the term is fulfilled
    setTimeout(resolve, ms);
    unknwonidentifier.someproperty // some error/exception here !!!!!!!!!!!!!!!!!!!!!!!!! <--- 
  });
};

timeout(12).then(dosomethingCool)

will not without SetPromiseRejectionTracker log you anything as the exception happens inside the Promise and there is no catch defined.

p.s. queue here is an internal to goja queue that gets ran through when the stack is empty, and before it returns to the host(k6) for example from calls to RunString. This also is part of the specification.

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we merge a partial solution? Do we expect that it's fixing the most common use-case where this issue is faced?

Comment on lines +315 to +316
// More complete implementation might need to wait for some time, the end of an iteration,
// some other event before logging a "rejection" that never got "handled"
Copy link
Contributor

@codebien codebien Jan 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the issue:

waiting some time or end of iteration seems like a good idea but it probably needs an "OOM" prevention to not wait on ... 1milltion unhandled rejections or something like that.

So, is this a real order of magnitude in the case we would track/registry all the "not yet handled" rejection? Seems so high to me but I expect to be my fault in terms of JavaScript world knowledge.

@na-- na-- added this to the v0.37.0 milestone Jan 12, 2022
@mstoykov
Copy link
Contributor Author

mstoykov commented Feb 4, 2022

In the end this will be part of #2228 with 295ffc0

@mstoykov mstoykov closed this Feb 4, 2022
@mstoykov mstoykov deleted the logUnhandledPromiseRejections branch February 4, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants