`multipleResolves` event should not be triggered when using `Promise.all()` or `Promise.race()`

[ehmicky]

Version: v11.1.0
Platform: Linux myhost 4.18.0-10-generic #11-Ubuntu SMP Thu Oct 11 15:13:55 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

When using Promise.all() or Promise.race(), several child promises might be legitimately rejected. This might be intended by the developer, so it should not trigger a multipleResolves process error. This is in contrast to calling reject() several times in new Promise() which definitely indicates a programming error.

'use strict'

process.on('multipleResolves', function() {
  console.log('This should not be called')
})

Promise.all([Promise.reject(), Promise.reject()])

Use case 1: a unit test is checking that an async function returns a rejected promise. The unit test fires the function several times in parallel for performance reason. The test expects each function call to reject a promise. I.e. multipleResolves should not be fired.

Use case 2: I created a library log-process-errors which logs every process errors (including multipleResolves). The goal is to make sure process errors are properly logged in production, so they can be fixed. However this does not work if Node.js is triggering them where there is no programming errors.

[BridgeAR]

@nodejs/v8 this looks like a bug in V8 to me. It should indeed not be triggered outside of a promise constructor.

[bmeurer]

Hm, I'm not 100% sure. Technically you are rejecting a promise multiple times here, so it's appropriate to fire the hook. But I can see how Promise.all() is somewhat special (and same for Promise.race()). So I'm happy to change this if Node folks think it should be different (please file a bug on the V8 issue tracker in that case).

[bmeurer]

Specifically for Promise.all() we would then silently swallow rejections again, which may or may not be desired.

[kanongil]

While it is likely possible to fix this specific issue at the v8 level, this does not seem like a v8 level issue.

Rather, the multipleResolves event is being misused for something other than debugging / diagnostics. It is not fair to assume that all code will no longer call resolve / reject multiple times, especially since any user-level workarounds will require extra system resources to track the state (which can not be extracted from the promise itself).

Really, this is should just be a documentation fix, that relabels the usage of the event for specific debugging purposes. The existing recommendation to end the process on any such events seem absurd, and poorly thought out.

[targos]

@bmeurer would it be possible/make sense to keep emitting the event but with another name so we can differentiate multiple rejects in the Promise executor (or in a thenable callback) vs. Promise.all/Promise.race?

[bmeurer]

Promise.all and Promise.race invoke the Promise constructor (under the hood). Could you just check the stack trace and figure out whether you're called from Promise.all or Promise.race?

[devsnek]

just checking if Promise.all and Promise.rsolve are on the stack isn't adequate. one could do Promise.all.call(FakePromiseWithProgrammingErrors)

on a much larger level, I think this proves that multiple resolves/rejects aren't actually always programming error, and shouldn't be treated as exceptional.

[kanongil]

on a much larger level, I think this proves that multiple resolves/rejects aren't actually always programming error, and shouldn't be treated as exceptional.

Yes, exactly, hence my documentation fix suggestion. This "feature" has been used for ages, e.g. see https://stackoverflow.com/q/20328073/248062 and https://stackoverflow.com/a/18218542/248062. V8 also uses it (as evidenced by this issue), I use it myself, and I'm sure others do as well.

[benjamingr]

on a much larger level, I think this proves that multiple resolves/rejects aren't actually always programming error, and shouldn't be treated as exceptional.

I think it might make sense to scope the hook to its original intent or throws after resolves/rejects in the promise constructor or resolves/rejects after throws.

[bmeurer]

Talking to @hashseed I see two possible ways:

1. Mark the promise created in Promise.all and Promise.race such that multiple resolve/reject calls will not be reported.
2. Limit the machinery to while the Promise constructor is running, i.e. only report accidental exceptions thrown from the executor.

cc @hashseed