Misleading unhandled rejection warning when using when.settle

[rooftopsparrow]

Hello!

I'm wondering if this is expected behavior?

// node repl with latest when
> when = require('when')
> when.settle([ Promise.reject('help'), Promise.resolve('yesss') ])
{ state: 'pending' }
> Potentially unhandled rejection [1] help (WARNING: non-Error used)
> when.settle([ when.reject('help'), when.resolve('yesss') ])
{ state: 'pending' }

Notice that we get a warning when using global Promises but we do not get that warning with when promises. It has caused some painful debugging to find that it wasn't actually unhandled.

Thanks for your time and library!

[rkaw92]

Hi, I've seen some reports like this in Node.js proper. Can you compare your output to the one that you would get if you left out any when.js references and replaced the when.settle with dummy functions?

[rooftopsparrow]

I'm not sure what dummy functions would accomplish, because if you don't handle the rejection you get this output:

> Promise.reject('help')
Promise { <rejected> 'help' }
> (node:27570) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 3): help

But I'd expect when to handle a rejected promise in the array. This function is more of what I would expect, but I could be wrong on the assumption

> const settle = (promises) => {
...   return Promise.all(promises.map( p => {
.....     return p.then( result => ({ state: 'fulfilled', result })).catch( error => ({ state: 'rejected', error }))
.....   }))
... }
> settle([ Promise.reject('help'), Promise.resolve('yesss') ]).then( settled => console.log(settled) )
Promise { <pending> }
> [ { state: 'rejected', error: 'help' },
  { state: 'fulfilled', result: 'yesss' } ]

Notice no warning output. Let me know if this is what you are looking for.

Thanks for your time!

[rkaw92]

Sorry, was on a mobile so the reply was rather on the brief side. The behavior difference test is what I had in mind (the warnings are similar but not the same). I've now run the code and indeed - the last version that does not produce the warning is 3.6.0. Strangely, it is a patch release (3.6.1) that introduces the warnings. I have run the test on Node 6.0.0 to eliminate any new Node.js issues that have been coming up lately.

Going to check what happened between 3.6.0 and 3.6.1...

[rooftopsparrow]

Some more information about my environment:

All output above was with when@3.7.7 and node@6.9.1. macOS Sierra 10.12.1

[rkaw92]

git bisect says it's due to commit 5f1512a . Not sure about the exact interaction going on down there - will have to look into the code and do some poking. This looks pretty reproducible, though.

[rkaw92]

It's right there in the middle of that commit - if I checkout 3.6.1 but change the implementation of settle back to what it was before the patch (and also restore mapArray from the previous version), the warning goes away. Trying to debug it now. Just minor conclusions at this moment, nothing that looks like a breakthrough:

• Also happens for bluebird Promise objects (does it use native ones? would have to check)
• Also happens with a timeout before the rejection - not a case of already-resolved promises breaking the warning

[rkaw92]

I have found the root cause - it's the creation of an extra Thenable that causes the warning to be printed. CC @briancavalier 'cause when.js Handler internals are at play here.

The flow in the newest when.js version is as follows:

• settleOne(p) wants to inspect the promise by calling Promise._handle(x) - this, however, creates a new Thenable (because p is a foreign promise). Since the Thenable is (by prototype) also a Pending, it has this._state = 0.
• The h.state() === 0 conditional passes (there are no "shortcuts" for foreign promises) and enters the first (positive) branch:
• toPromise(p) is called, which is just an alias for when's Promise.resolve, which in turn creates a new promise. On the same line, two handlers are registered with .then(). This makes the generated promise handled properly, and causes it to not emit warnings.

However, what about our first Thenable? It will be resolved (by rejecting) when the original foreign Promise rejects (because there is an AssimilateTask that "connects" the untrusted "thenable" - the foreign promise - to the new Thenable's fate). When that happens, a new Rejected(x) will be generated in Pending.prototype.reject. This Rejected instance is the one which emits the warning, because:

• Its fate was bound to the original foreign promise
• It immediately calls _report() on itself in the constructor
• Nobody can handle it because it's not a promise (technically, it's not Pending)
• Nobody could have _unreport()ed it because it did not exist at the time the Thenable was created

Whew, that was a tricky one. This bug has only shown up because the function settleOne, in array decorators, messes with parts in makePromise.js which are labeled as "Promise internals". Probably not a good idea.

Also note that the h._unreport() in settleOne does not help for 2 reasons - it's in the wrong branch and it won't work on a Pending, anyway (since Pending.prototype._unreport has a safety check for resolved handlers only).

I'll try and come up with a clean fix for settleOne. It's probably best to try and get to the underlying Handler instance to get its .state(), but without creating a new Handler ever. In case we can't optimize this way (i.e. we're dealing with an unresolved or un-Promise-like value), we'll be re-using the same branch as if it's pending, and wrapping it in a new Promise, anyyway.

[rkaw92]

Got it. Pull request #494

[rkaw92]

@rooftopsparrow can you confirm that this issue is fixed for you on master?

[rooftopsparrow]

Looks great!

> when.settle([ Promise.reject('help'), Promise.resolve('yesss') ])
{ state: 'pending' }
> p = _
{ state: 'fulfilled',
  value:
   [ { state: 'rejected', reason: 'help' },
     { state: 'fulfilled', value: 'yesss' } ] }
> when.settle([ when.reject('help'), when.resolve('yesss') ])
{ state: 'pending' }
> p = _
{ state: 'fulfilled',
  value:
   [ { state: 'rejected', reason: 'help' },
     { state: 'fulfilled', value: 'yesss' } ] }

Output above is on node v6.9.1. macOS Sierra 10.12.1 and on commit facc6ba