premature doEmitWarning Unhandled promise rejection

[johngrabner]

• Version: v8.9.1
• Platform: Windows 10
• Subsystem: warning.js / doEmitWarning

Warnings for unhandled promise rejections should only be declared after the last reference to promise is removed and not during tick handling as is currently done.

The following example should not generate a warning. p rejection is handled.
"use strict";

let p = Promise.reject()

setTimeout(()=>{
p.catch(
()=>{console.log("caught")})
}, 1000)

Only after all references to p are gone, does node know the rejection was not handled.

Debugger listening on ws://127.0.0.1:43271/d7e878b8-45d9-4c4f-98e3-d760fb7b8074
(node:9480) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): undefined
warning.js:18
(node:9480) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
warning.js:18
(node:9480) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)

---

here is a more realistic example

"use strict";

function promise_to_do_something(i){
if(i == 0){
return Promise.reject("i don't like 0's")
} else {
return new Promise((resolve, reject)=>{
// emulate doing something
setTimeout(()=>{
if (Math.random()<0.5){
reject()
} else {
resolve()
}
}, 1000)})
}
}

promise_to_do_something(0)
.then(()=>{ console.log("they all passed")})
.catch(()=>{console.log("handle rejection")})

the above generates a warning while debugging, but there is no reason for this.

---

a cludge around this node bug, is to always delay before calling Promise.reject

"use strict";

function promise_to_do_something(i){
if(i == 0){
// I want to reject now, but node prematurely complains, so delay my rejection
return new Promise ((resolve, reject)=>{
setTimeout(()=>{ reject("i don't like 0's")})
})
} else {
return new Promise((resolve, reject)=>{
// emulate doing something
setTimeout(()=>{
if (Math.random()<0.5){
reject()
} else {
resolve()
}
}, 1000)})
}
}

promise_to_do_something(0)
.then(()=>{ console.log("they all passed")})
.catch(()=>{console.log("handle rejection")})

---

if you think the above cludge is ok, then the following example is most realistic and the cludge does not work.

"use strict";

function promise_to_do_something(i){
if(i == 0){
// I want to reject now, but node prematurely complains, so delay my rejection
return new Promise ((resolve, reject)=>{
setTimeout(()=>{ reject("i don't like 0's")})
})
} else {
return new Promise((resolve, reject)=>{
// emulate doing something
setTimeout(()=>{
if (Math.random()<0.5){
reject()
} else {
resolve()
}
}, 1000)})
}
}

let p = []
for (let i=0; i<10; i++){
p.push(promise_to_do_something(i))
}
Promise.all(p)
.then(()=>{ console.log("they all passed")})
.catch(()=>{console.log("handle rejection")})

---

The cleanest solution is for node to complain about unhandled promise rejection if references to the promise are none existing (ie: when last reference is deleted). This is the only way node knows a promise rejection will not be handled. This is clean from a semantic point of view.

[johngrabner]

Here is another side of the bug. Here the debugger complains that the second rejection is not handled, but it was earlier, before the second call. This odd behaviour would be solved if the suggested fix is implemented (ie: warn and complain only when/if last reference being deleted).

"use strict";

Promise.all([
new Promise((resolve, reject)=>{setTimeout(()=>{reject()},1)}),
new Promise((resolve, reject)=>{setTimeout(()=>{reject()},10)})
])
.catch(()=>{console.log("caught")})

[devsnek]

I don't think this is really something that can be done. IIRC the only way to check ref counts would be PersistentBase::IsNearDeath but we don't have persistent handles of every promise created because that would be a pretty big overhead.

[TimothyGu]

#15126 would fix this, but the performance overhead and GC unpredictability make it unfit for general usage at this moment.

This is definitely a known problem, and ad-nauseam discussion has been conducted in that PR, in #12010, and in all referenced issues. Closing this issue as such.

[TimothyGu]

An oft-cited workaround is this:

"use strict";

let p = Promise.reject()
p.catch(() => {}); // <<

setTimeout(()=>{
  p.catch(
    ()=>{console.log("caught")})
}, 1000)

[benjamingr]

This is not a problem, this is a design choice, the alternative has false negatives and the false positives are better. This is why we have a rejectionHandled event.