JavaScript Promises – Error Handling Inside Promise.all

error handlingjavascriptpromises

I'm looking at this piece of code and whilst it works, it feels wrong to me. I'm wondering if I am looking at an anti-pattern?

The code starts with two promises, getAccounts and getTransactions. On one of the promises there is a catch block that handles and error but returns a resolved promise with an object that looks friendly.

let accounts = getAccounts()
                   .catch(error => Promise.resolve({errorCode: M011}));

let transactions = getTransactions();

These are then used in a Promise.all.

Promise.all([accounts, transactions])
       .then(results => createStatement(results));

This looks wrong to me because on a failure of getAccounts I thought the Promise.all should fail fast. However on the other hand the failure of getAccounts is handled as early as possible which, surely, is a good thing? The only thing about it is that it doesn't return with another Promise.reject

The rationale given was that this way the Promise.all doesn't choke. Of course another question entirely is why is this piece of code using Promise.all in the first place? That I don't know, legacy code.

Best Answer

This is not an anti-pattern. This is a code smell, which is to say that this is the sort of thing you should avoid when it's feasible to do so, but there may be legitimate cases where nothing else does the job.

I thought the Promise.all should fail fast.

Yup. This is exactly why the default behavior of Promise.all is to fail fast.

The rationale given was that this way the Promise.all doesn't choke.

I assume that means you're in a situation where even if some of the promises reject, you can still do something useful with the other promises that resolved successfully. This is definitely not an uncommon situation.

Of course another question entirely is why is this piece of code using Promise.all in the first place?

Presumably because that useful thing you can do with however many promises did resolve can only be done...after they've all resolved.

Let's pretend for a moment that the promises represent bank transactions and your UI is going to display either green or red text based on whether the transaction succeeds.

If only some transactions succeed and others fail, you would definitely want to show the successful ones in green and the failed ones in red, rather than "choking" and settling for a single frustratingly uninformative "one or more of your transactions may have failed" message. In that context, I can't imagine anyone objecting to code like this:

let resultRowPromise = executeTransactions(desiredTransactions)
           .then(result => Promise.resolve({ color: "green", message: "Success =D" }))
           .catch(error => Promise.resolve({ color: "red",   message: "Fail ;_;" }));

which we might "loop over" with a Promise.all at some point.

This is a case where it makes sense for the underlying promise to reject, it makes sense to wait for a set of such promises to finish, and it makes sense to handle failures on a per-promise basis instead of treating all failures as a failure for the whole batch.

But if any of those is not the case in your getAccounts() example, then you should consider alternatives such as making the underlying promise not reject in the first place, reacting to each promise resolution individually instead of waiting on a Promise.all, or simply letting Promise.all fail fast if in your case it is totally okay to ignore the successful ones.

In fact, even the example I just gave doesn't necessarily meet those standards. It's likely you could also handle each transaction promise individually, so the user sees "Transaction 1 succeeded!" and then a few seconds later "Transaction 2 failed!". You could then do a Promise.all over the promises for rendering these messages (which should always succeed unless there's a rendering error) so that you can put some additional message like "All transactions succeeded!" or "3/10 transactions failed!" at the very top.

When thinking about how promises propagate results and errors, I find it often helps to consider the equivalent "normal", "synchronous" or "imperative" non-promise code that returns values and throws/catches exceptions. For your example, that would probably be:

var results = [];
for(let i = 0; i < accountIds.length; ++i) {
    try {
        var account = getAccount(accountIds[i]);
        results.push(account);
    } catch(e) {
        results.push({ errorCode: M011 });
    }
}

Everything I said above applies to this code as well, though seeing it like this might make it more obvious why it applies. To summarize: this might be legitimate, but it definitely needs justification, and it's definitely worth thinking about whether that justification holds up or if there are cleaner alternatives.

Related Topic