Comment by moring | Hacker News Reader

Comment by moring | Hacker News Reader

Parent Full thread

One thing I have recently stumbled upon is that the way Promise.all() is commonly used is "wrong" in the sense that it is prone to uncatchable promise rejections (which cause both Node and Deno to exit the whole process by default).

I would really like to see a tutorial on how to solve this the right way -- right now it seems that everybody does it the "wrong way" and the problem isn't even mentioned.

Common pattern #1 (tuple-style):

  const result = await Promise.all([
   someAsyncFunction1(computeArg1()),
   someAsyncFunction2(computeArg2()),
  ]);

Common pattern #2 (array-map-style):

  const result = await Promise.all(elements.map(x => someAsyncFunction3(computeArg3(x))));

Both suffer from the same problem: If computeArg2 / computeArg3 potentially throw a (synchronous) error, then some promises have already been created, but Promise.all() never runs and so they don't have a catch-handler. If one of those rejects ("throws asynchronously") then that rejection is unhandled and crashes the process.

The problem is also discussed here, but in the context of "should ECMAScript be changed": https://es.discourse.group/t/synchronous-exceptions-thrown-f...

The answer is to never throw exceptions from promise returning functions. Mark all promise returning functions `async` - even when it doesn't `await` - because an async function always returns.

https://typescript-eslint.io/rules/promise-function-async/

This only solves the second example (the "map" one), but not the first example because it doesn't contain any promise-returning function that throws.

> doesn't contain any promise-returning function that throws

Then what is the issue described in #1?

Ahh, the compute arg calls

That one is super interesting. For the first example, I'm not even really sure how I'd expect that to behave, it feels like no matter how it works it becomes slightly inconsistent with my expectations of the language. Like, if it were changed to somehow let a .catch grab the synchronous errors or have it somehow evaluate all of the compute functions synchronously first before making the promises, it still feels wrong. Maybe the only good solution is run the compute args separately beforehand?

For the second example, it's kind of a weird one, but I'm actually a fan of using async .reduce to accomplish that. It took me a minute the first time I saw someone do it, but I think it more cleanly solves the problem and gives you more control over the result.

How would you use reduce for that?

Reduce can use an async function which wraps the collector in a promise and gives you a ton of control over how it executes because you can choose at what point you want the function to block waiting for previous results.

In this particular case it's a lot more verbose, obviously, but if you're doing any kind of further processing of each element, you can move it into the reduce and be able to control exactly what you get out of it if something fails for any reason, while still running mostly concurrently like all or allSettled would.

  const result = await elements.reduce(async (collector, x) => {
        const arg3 = computeArg3(x) // explicitly handle whatever errors, try catch, whatever you need
        const asyncResult = await someAsyncFunction3(arg3)

        // this has to happen after calling someAsyncFunction because it will block until the previous iteration finishes
        collector = await collector;

        collector.push(result)

        return collector
  }, []);

In Scala there's Future.delegate for turning sync failures into failed futures. Seems like something equivalent should be possible in Javascript.

Promise.allSettled(…x) :)

This doesn't solve the issue at all, does it? If Promise.all() is never reached, neither is Promise.allSettled().

allSettled() returns an array of objects for each promise result that tells you if it has been rejected or resolved, so as long as all the promises reject or resolve then it should solve the problem I think?

moring is describing an issue where the creation of the promise itself fails, which means Promise.all/Promise.allSettled is never called.

Use the second pattern with `.allSettled` but map it with an async function.

      const result = await Promise.allSettled(elements.map(async x => someAsyncFunction3(computeArg3(x))));

Yep, that works fine: https://www.typescriptlang.org/play?target=8#code/IYZwngdgxg...

Yes, it works fine. As mentioned below just pass an asynchronous function and it’ll catch it.

Reply on news.ycombinator.com