2

We added an event handler for "unhandledRejection" in our node application, this handler terminates the process (as node says will be the default soon).

However we're seeing what appears to be superfluous (or at least premature) events.

We're using Q.allSettled (which handles rejected promises), I see in the debugger that the promise array has two elements, the first rejected and the second pending (it will be resolved). After placing breakpoints in the unhandledRejection handler and in the then following the allSettled, I get the unhandledRejection first and immediately afterwards the then (so the rejection is handled).

The documentation for unhandledRejection says (emphasis mine):

The 'unhandledRejection' event is emitted whenever a Promise is rejected and no error handler is attached to the promise within a turn of the event loop.

My assumption is that the mechanism that created the array of promises blocked between the first promise being rejected and the creation of the array but I haven't been able to reproduce this in a toy example.

The best minimal example I've been able to come up with is a bit contrived but I think it has the same root cause as our code. Am I missing something here? How can node make unhandledRejection terminate the process by default if this is possible?

process.on("unhandledRejection", (reason, p) => {
    console.error(`### Got unhandled rejection: '${reason}'`);
});


function doesNotFireUnhandledRejection() {
    let rejected = Promise.reject('an error');
    return new Promise((resolve, reject) => {
        resolve(rejected);
    })
    .catch(err => console.log("Caught:", err));
}


function firesUnhandledRejection() {
    let rejected = Promise.reject('an error');
    return new Promise((resolve, reject) => {
        setTimeout(() => resolve(rejected), 0);
    })
    .catch(err => console.log("Caught:", err));
}

BTW, The same behaviour happens with setImmediate but not with process.nextTick (although I could swear that last week process.nextTick did cause the event to fire...).

Motti
  • 110,860
  • 49
  • 189
  • 262
  • I know that you are trying to create a min example to reproduce it, but what's the reason for statically rejecting a promise, getting back a `promise` object, and then wrap it inside the `resolve` of another promise? – quirimmo Dec 23 '18 at 15:45
  • @quirimmo, there is none. I tried to create a more meaningful example but haven't been successful (yet?) – Motti Dec 23 '18 at 15:50
  • 1
    @blex, not if you handle the rejection as you can see from the `doesNotFireUnhandledRejection` example – Motti Dec 23 '18 at 15:50
  • ah ok I think I got your question now. You mean, why that first rejection is not catched if you introduce a timeout? – quirimmo Dec 23 '18 at 15:51
  • 1
    @blex in real life the function that created the original rejection is way downstream and for most callers returning a regular rejection is fine. There's no reasonable reason to change it (and all its clients) to use wrapped promises. – Motti Dec 23 '18 at 15:52
  • @quirimmo, it **is** caught (the _Caught:_ log line is printed) but before it gets the chance to be caught node reports an `unhandledRejection`. – Motti Dec 23 '18 at 15:54
  • but in your example, why not using some code like the following for example? `var rej = Promise.reject('an error'); new Promise((resolve, reject) => { return rej.catch(err => setTimeout(() => resolve(err), 2000)); }) .then(data => console.log('data:', data)) .catch(err => console.log("Caught:", err));` – quirimmo Dec 23 '18 at 16:01
  • @quirimmo, correct me if I'm wrong but by catching the error and then `resolve`ing it, you get a resolved `Promise` and not a rejected one. – Motti Dec 24 '18 at 07:13

2 Answers2

3

This is how process can be terminated on unhandled rejection (expected to be default Node behaviour in future versions):

process.on('unhandledRejection', err => {
  console.error(err);
  process.exit(1);
});

Rejected promise should be chained with catch(...) or then(..., ...) on same tick, otherwise it is considered unhandled rejection.

In rare cases when it's guaranteed that a rejection is handled later, a promise can be chained with dummy catch to suppress the detection of unhandled rejection:

function firesUnhandledRejection() {
    const rejectedPromise = Promise.reject('an error');
    rejectedPromise.catch(() => {});

    return new Promise((resolve) => {
        setTimeout(() => resolve(rejectedPromise), 0);
    })
    .catch(err => console.log("Caught:", err));
}
Estus Flask
  • 206,104
  • 70
  • 425
  • 565
  • Wow, by random chance I saw this answer just _after_ awarding a +500 bounty for an inferior answer on [my very similar question](https://stackoverflow.com/q/53869592/35070). Mind posting your answer there so I can accept it there? I promise that there's also gong to be a bounty. Also, I assume the second line should read `() => {}`, or maybe `e => {}`, shouldn't it? – phihag Dec 23 '18 at 16:25
  • 1
    Thanks for noticing, this was a typo. Actually, I'm not really sure if it's best solution for your case. Usually I'd go with Promise.all as it's closer to what's really going on since there are are two concurrent routines. I'll post a reply later. – Estus Flask Dec 23 '18 at 16:38
  • The problem is that a function that returns a rejected `Promise` can't know when and how the returned `Promise` will be used. So I don't see how you can decide when to use the _dummy catch_ method, additionally by using the _dummy catch_ method you lose the ability to diagnose truly un-handled rejections. – Motti Dec 23 '18 at 18:45
  • @Motti `.catch(() => {})` is supposed to be used inside `firesUnhandledRejection`, you already know that a rejection will be handled because you handle it with another `catch`. Notice that functions in your question don't return rejected promises so they don't show the situation you're describing. Please, provide an example that replicates your case more closely. – Estus Flask Dec 23 '18 at 19:26
2

To answer your question:

  • Things that have microtask semantics (process.nextTick, then handlers, continuations of async functions and enqueueMicrotask) happen before unhandled rejection checking.
  • Things that have macrotask semantics (timers, i/o) happen after unhandled rejection checking.

That's what we mean by: "within a turn of the event loop".

Node.js is behaving as expected in this case. Note that in your case:

function firesUnhandledRejection() {
    let rejected = Promise.reject('an error'); // < -- this is the unhandled rejection
    return new Promise((resolve, reject) => { // <- this promise is totally fine
        setTimeout(() => resolve(rejected), 0); // <- you associate them too late
    })
    .catch(err => console.log("Caught:", err));
}

So the problem is that for too long (timers fire) there is a rejected promise without a error handler.

Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • This is pretty much how I understood things too but it doesn't answer the question. If some legitimate flows cause rejections to be handled after a tick has passed how can we detect _true_ unhandled rejections and how can we prevent node from exiting when such a case happens? – Motti Dec 23 '18 at 18:47
  • 1
    @Motti I'll be blunt - the unhandledRejection event and its timing is opinionated (I made it that way). Since detecting all true unhandled rejections is effectively the halting problem Node makes the informed decision to discourage asynchronous handling of rejections. Personally I avoid the promise constructor altogether by the way. We had GC based unhandled rejection detection but that won't happen anytime soon. – Benjamin Gruenbaum Dec 24 '18 at 10:21
  • If you want to avoid false positives we also expose a 'rejectionHandled' event to detect 'false positives'. – Benjamin Gruenbaum Dec 24 '18 at 10:22
  • Thanks! This goes a good way to answering a question I haven't asked, namely can I detect if a promise is unhandled after a certain time. I can use `unhandledRejection` to create a set of rejections and `rejectionHandled` to remove those that are handled before the timer elapses. Then I can terminate the process if a rejection is really not handled. – Motti Dec 24 '18 at 12:13
  • 1
    @Motti the hard part is - how do you know if a rejection isn't handled? By the way - we're very open to ideas and feedback for how to move forward with this in Node. The deprecation warning isn't entirely correct btw. – Benjamin Gruenbaum Dec 24 '18 at 12:17
  • 1
    Other than waiting for an arbitrary time I don't know (obviously not a good idea). If I think of something I'll be sure to let you know. – Motti Dec 24 '18 at 12:24