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

[hashseed]

I agree with @benjamingr that we should limit our scope to solve the original problem, which is to detect rejections after the promise has been resolved, through exceptions in the Promise constructor.

For that, we could simply check whether the Promise has been already resolved when we catch an exception inside the Promise constructor. The implementation would be fairly clean and local to the Promise constructor.

Option (2) suggested by @bmeurer is a bit broader than that, and would also report multiple calls to the resolve or reject closure. Distinguishing these calls from within the Promise constructor and from outside, which e.g. is the case with Promise.all, requires more logic and would make the Promise implementation even more complicated. And I'm a bit doubtful this is actually worthwhile.

Opinions?

[BridgeAR]

I personally would like a third option:

Mark the promise created in Promise.all and Promise.race such that it triggers a new event with which the hook is able to provide the origin as well. This way users could decide on their own what to listen to and what not.

It is definitely beneficial to know what promises were rejected in a Promise.all call.

[benjamingr]

@BridgeAR that sounds interesting (providing the users with more information) although unrelated to why we added the hook (it's a nice idea though).

@hashseed

For that, we could simply check whether the Promise has been already resolved when we catch an exception inside the Promise constructor. The implementation would be fairly clean and local to the Promise constructor.

One thing to keep in mind is that a throw - before - reject is also possible a-la:

new Promise(resolve => {
  setTimeout(resolve, 1000);
  cleanup(); // synchronous, but throws exception
})

That's not the "more important" case but it's also interesting to address.

[kanongil]

For those that argue that all uses of multiple resolves is a very bad thing, I would like to present a simple timeout use-case:

const fs = require('fs');

let promise = new Promise((resolve, reject) => {
  setTimeout(reject, 1000);

  fs.readFile(path, (err, data) => err ? reject(err) : resolve(data));
});

In this case, not having to worry about multiple resolves saves a lot of code.

[hashseed]

Option 2 suggested above solves this issue, right?

[benjamingr]

I would like to present a simple timeout use-case

I'd argue that example conflates the timeout logic with the reading file logic in the promise constructor. It also rejects with undefined which is unfortunate since there is no stack trace given.

const fs = require('fs').promises;
const delay = require('util').promisify(setTimeout);
let promise = Promise.race([
  fs.readFile(path),
  delay(1000).then(() => { throw new Error('timed out'); })
]);

[ehmicky]

It seems like several solutions were suggested, did you reach a consensus?

[Damaged-Organic]

+1 for fix on this. Multiple Resolve is something that considered to be a severe programmer error, while for Promise.all()/Promise.race() it is an expected behavior. multipleResolves is not triggered if 2 or more promises in the list resolve successfully, and so it should not be triggered if 2 or more reject.

[Jamesernator]

I don't think any action should be taken here until cancellation of Promises is actually worked out. Both the Promise.race and new Promise examples with delay(1000) suffer an identical problem of keeping the process alive until both promises have resolved even though only the result of one of them is actually needed.

With cancellation there could be something like:

import { promises as fs } from "fs"

const signal = new CancelSignal()

let fileRead = Promise.race([
  fs.readFile(path, { signal }),
  delay(1000, { signal })
], { onResolved: () => signal.cancel() })

Then Promise.race/Promise.all simply could just keep causing multipleResolves.

Note that in order for this to allow for warning (or ending the process) on multipleResolves it would be critically important that Promise.race/Promise.all must not cause multipleResolves if the promises passed into Promise.race are already settled and must call onResolved synchronously so that the other promises can be given a chance to never bother resolving (e.g. by entering a new third Promise cancelled state).

[devsnek]

You can already do this (finally pretty much exists for cleanup)

const timer = delay(5000)
Promise.race([timer, readFile])
  .finally(() => cancel(timer));

[Jamesernator]

You can already do this (finally pretty much exists for cleanup)

That doesn't solve the issue of my original point. My point is that the reason observing multipleResolves at all is it implies you were doing work that wasn't necessary.

But Promise.race/Promise.all also fire multipleResolves which is actually fine because they're indicative of the exact same error (which is continuing work when you could've ceased).

However multipleResolves only makes sense for tracking this if there's an opportunity in Promise.all/Promise.race to cleanup before any other resolve/reject could happen. This means that in order to be even able to track the superfluous work cancellation must be synchronous to avoid triggering multipleResolves.

But then this is only useful if everything that returns a Promise is cancellable including readFile and so forth. Now it's not necessarily blocking on the cancellation proposal but if there's no way to standard way to distinguish between a "cancelled" Promise and one that resolved normally (e.g. third Promise state or standardized CancelError) then multipleResolves can't really be distinguished between actual cancellations and non-cancellations.

Personally I think without synchronous cancellation of everything the multipleResolves hook is kinda pointless, it'll either be arbitrarily inconsistent if Promise.all/Promise.race are changed (because it's still the exact same class of errors with those methods) or false positives will be too easy.

[devsnek]

To be fair, all of the promise events we have are only useful if you make assumptions about the usage patterns of promises, not just multipleResolves. As is proved with Promise.all and such, you need a human to tell whether or not the event is actually indicative of a bug. To this end, I think it's more important to have a good interface for these events, rather than blindly treating their existence as erroneous. Get a list of events triggered, maybe some integration with devtools for jump to source and whatnot, include async context and stack, and you have a pretty useful async debugging tool.

[Jamesernator]

As is proved with Promise.all and such, you need a human to tell whether or not the event is actually indicative of a bug.

I agree but unfortunately Node's docs give quite possibly the worst advice I've ever seen: terminate the process on any multipleResolves.

There doesn't really seem to be a coherent design over these things other than "hey here's some stuff v8 exposes so terminate your program if they happen".

I grow tired of bringing it up again and again but I do feel like Node's "design" is comically terrible for actually tracking errors that I feel I need to. But most of these problems are easily solvable by something like Zones as this gives you narrower contexts in which to observe specific errors rather than just the whole program.

With something like Zones you could observe unhandledRejections/multipleResolves within your own library or even as narrow as specific functions or calls and decide whether or propagate them or not.

Hell I even think something like Zones would be a viable way of exposing propagated cancellation too which would make these sort've things even simpler again.

But instead of something with a coherent design like Zones we get madhouse advice of terminating the process when doing common things Promises are explicitly designed to from anywhere in the process.

[Jamesernator]

Frustratingly as I have just remembered Node.js already has this Zone-esque thing and it's called domains, which are deprecated for reasons not specified in the docs.

Why is this deprecated? It appears that it could be extended to solve these problems that these hooks introduce.

[addaleax]

Why is this deprecated?

Some info on that topic is in https://nodejs.org/en/docs/guides/domain-postmortem/

[devsnek]

zones got me curious so i made this https://gist.github.com/devsnek/07a61aec7d9f40d1495048790dd30a8f

[Jamesernator]

zones got me curious so i made this https://gist.github.com/devsnek/07a61aec7d9f40d1495048790dd30a8f

Nice.

I wasn't even aware of async_hooks, man that's pretty close to a full zone solution (currently Promise.reject(12) that isn't awaited doesn't trigger unhandledRejection for some reason).

[murrayju]

Amazed to discover that this is still an unsolved issue.

I just want to mention another possible workaround, depending on your use case. You can use Promise.allSettled() (which doesn't have this multipleRejections problem), followed by a map. Something like this:

(await Promise.allSettled([...])).map((r) => {
  if (r.status === 'rejected') {
    throw r.reason;
  }
  return r.value;
})

This almost gives me what I want from Promise.all. It has the problem of continuing to wait for all of the promises, even if one has already rejected. What I'd really expect is for it to behave like Promise.race such that it gets rejected with the first inner rejection (but waits for all of them to resolve in the happy case).

[SimonSimCity]

Just to add a note here from a user of Node.js: To me there are 3 cases we need to consider here:

1. A developer who doesn't care if the event multipleResolves is triggered.
2. A developer deeply cares about the event multipleResolves and uses it to spot possible bugs in his code (accidentally resolving a promise multiple times).
3. A function implemented in the core, available in user-space, which internally triggers the event multipleResolves.

I'm not here to define if 1. or 2. is a bad design or anything in this direction, but I want to say that 3. might not be a the best case for a developer who identifies himself by the 2. description.

Moreover, this might be best caught in user-space, where one would use a custom Promise wrapper for cases, where he considers multiple resolves as unwanted. Mainly because other libraries could be written by a developer identifying himself as 1. whereby you anyways will get a lot of spam in your logs.

Note: But I hope agree that triggering this event in a function, which called Promise.race() - which, by its name, you'd only expect to focus on the one which finished first, sounds weird. Promise.all() is also a bit interesting that it ends on the first failed promise - but that's why we now have Promise.allSettled().

[Mesteery]

I think we can close this since 'multipleResolves' is deprecated: #41872.

[ehmicky]

Thanks @Mesteery and @benjamingr 👍