2

I discovered a weird issue while trying to implement a function that awaits all promises of an array and catches all errors. This implementation led to a complete application crash caused by an unhandled promise rejection.

const completeAllPromises = async (promises) => {
    const errors = []

    for (let i = 0; i < promises.length; i++) {
        await promises[i].catch((error) => {
            error.message = `${i}: ${error.message}`
            errors.push(error)
        })
    }

    if (errors.length) return Promise.reject(errors)
}

const timeout = (duration) => {
    return new Promise((resolve) => setTimeout(() => resolve()), duration)
}

const fail = async () => {
    return Promise.reject(new Error('Failed'))
}

const crash = async () => {
    await completeAllPromises([timeout(100), fail()]).catch(
        (error) => console.log(`Crash prevented, catch promise rejection`, error), // never be called
    )
}

const main = async () => {
    // will crash and catch will not be called
    await crash().catch((error) => console.log('Finished crash() and catch error', error))
}

main()

It seems like the catch implementation in completeAllPromises function is executed at runtime, so while awaiting the first timeout promise, fail() will reject and not be catch. The weird thing for me is that the crash() and also not main() will catch this error. It almost seems like the call chain is not resolvable at the time fail() rejects.

Switching timeout(100) and fail() will result in a properly catch errors. Using a try and catch block will also lead to the same issue.

Solution ✅

const completeAllPromises = async (promises) => {
    const errors = []
    const results = await Promise.allSettled(promises)
    results.forEach((result) => {
        if (result.status === 'rejected') errors.push(result.reason)
    })
    if (errors.length) return Promise.reject(errors)
}

const timeout = (duration) => {
    return new Promise((resolve) => setTimeout(() => resolve()), duration)
}

const fail = async () => {
    return Promise.reject(new Error('Failed'))
}

const crash = async () => {
    await completeAllPromises([timeout(100), fail()]).catch(
        (error) => console.log(`Crash prevented, catch promise rejection`, error), // never be called
    )
}

const main = async () => {
    // will crash and catch will not be called
    await crash().catch((error) => console.log('Finished crash() and catch error', error))
}

main()
Patrick
  • 67
  • 1
  • 9
  • 1
    why dont you use allSettled (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled) instead ? – Ji aSH Jan 11 '22 at 09:27
  • 2
    "*runs an array of promises*" doesn't make sense. Promises cannot be executed, they can only be observed. As soon as you call `timeout(100)` and `fail()`, the asynchronous tasks are already running - before you pass anything to `completeAllPromises`. – Bergi Jan 11 '22 at 09:32
  • 2
    See See https://stackoverflow.com/questions/46889290/waiting-for-more-than-one-concurrent-await-operation and https://stackoverflow.com/questions/45285129/any-difference-between-await-promise-all-and-multiple-await for why you get the unhandled promise rejection. Maybe also https://stackoverflow.com/q/59694309/1048572 – Bergi Jan 11 '22 at 09:35
  • 2
    It's **really** subtle, but it is fundamentally the issue Bergi points to above. Here's how it happens: You call `timeout` and `fail` and pass the resulting promises into `completeAllPromises`. That function starts a loop, calls `catch` on the promise from `timeout`, and awaits the result. At that point, nothing is handling rejection of the promise from `fail`, and the stack unwinds (waiting for the timeout) and returns to platform code (Node.js's internal event loop code). The platform code sees that `fail`'s promise has been rejected and the rejection has not been caught (yet). You could... – T.J. Crowder Jan 11 '22 at 09:42
  • 1
    ...fix it by removing the `await` in the loop and instead storing the promises you get from `catch` in an array, then `await Promise.all(thatArray)` before doing your `return`. But as @JiaSH pointed out, there's already a tool for this: `Promise.allSettled`. – T.J. Crowder Jan 11 '22 at 09:45
  • 1
    (I should have said above: You **do** end up handling the rejection from `fail`, it's just that you don't do it before the unhandled rejection detection kicks in, because you don't do it until after you `await` the `timeout` promise.) – T.J. Crowder Jan 11 '22 at 09:47
  • 1
    Separately: I **strongly** urge you not to combine `async`/`await` syntax with `.catch` method calls, it's really hard to read. If you want to catch promise rejection in an `async` function, use `try`/`catch` around the `await` instead (or an existing promise combinator like `Promise.allSettled`). – T.J. Crowder Jan 11 '22 at 10:02

0 Answers0