Hacker News

Unless it changed how NodeJS handles this you shouldn't use Promise.all(). Because if more than one promise rejects then the second rejection will emit a unhandledRejection event and per default that crashes your server. Use Promise.allSettled() instead.

vinnymac 3 days ago [ - ]

Promise.all() itself doesn't inherently cause unhandledRejection events. Any rejected promise that is left unhandled will throw an unhandledRejection, allSettled just collects all rejections, as well as fulfillments for you. There are still legitimate use cases for Promise.all, as there are ones for Promise.allSettled, Promise.race, Promise.any, etc. They each serve a different need.

Try it for yourself:

> node

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

> Promise.reject()

> Promise.allSettled([Promise.reject()])

Promise.allSettled never results in an unhandledRejection, because it never rejects under any circumstance.

kaoD 3 days ago [ - ]

This didn't feel right so I went and tested.

    process.on("uncaughException", (e) => {
        console.log("uncaughException", e);
    });

    try {
        const r = await Promise.all([
            Promise.reject(new Error('1')),
            new Promise((resolve, reject) => {
                setTimeout(() => reject(new Error('2'), 1000));
            }),
        ]);

        console.log("r", r);
    } catch (e) {
        console.log("catch", e);
    }

    setTimeout(() => {
        console.log("setTimeout");
    }, 2000);

Produces:

    alvaro@DESKTOP ~/Projects/tests
    $ node -v
    v22.12.0

    alvaro@DESKTOP ~/Projects/tests
    $ node index.js 
    catch Error: 1
        at file:///C:/Users/kaoD/Projects/tests/index.js:7:22
        at ModuleJob.run (node:internal/modules/esm/module_job:271:25)
        at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:547:26)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5)
    setTimeout

So, nope. The promises are just ignored.

panzi 3 days ago [ - ]

So they did change it! Good.

I definitely had a crash like that a long time ago, and you can find multiple articles describing that behavior. It was existing for quite a time, so I didn't think that is something they would fix so I didn't keep track of it.

kaoD 3 days ago [ - ]

Perhaps you're confusing it with something else? I tried down to Node 8 (2017) and the behavior is still the same.

Maybe a bug in userspace promises like Bluebird? Or an older Node where promises were still experimental?

I love a good mystery!

panzi 2 days ago [ - ]

Weird. Also just tried it with v8 and it doesn't behave like I remember and also not like certain descriptions that I can find online. I remember it because I was so extremely flabbergasted when I found out about this behavior and it made me go over all of my code replacing any Promise.all() with Promise.allSettled(). And it's not just me, this blog post talks about that behavior:

https://chrysanthos.xyz/article/dont-ever-use-promise-all/

Maybe my bug was something else back then and I found a source claiming that behavior, so I changed my code and as a side effect my bug happened to go away coincidentally?

kaoD 2 days ago [ - ]

It could be this: https://stackoverflow.com/questions/67789309/why-do-i-get-an...

If you did something like:

    const p1 = makeP1();
    const p2 = makeP2();

    return await Promise.all([p1, p2]);

It's possible that the heuristic didn't trigger?

cluckindan 3 days ago [ - ]

Typo? ”uncaughException”

kaoD 3 days ago [ - ]

Oops, thanks! Should've seen the default handler anyways if it was really uncaught.

mijkal 3 days ago [ - ]

When using Promise.all(), it won't fail entirely if individual promises have their own .catch() handlers.

andrewmcwatters 3 days ago [ - ]

Subtle enough you’ll learn once to not do that again if you’re not looking for that behavior.