1

I have the following code

await doAllCats();
await doAllDogs();
console.log("Finished processing Cats and Dogs ")

function doAllCats() {
  let promiseArray = [];
  for(let cat of cats) {
    promiseArray.push(doOneCat(cat));
  }
  return Promise.all(promiseArray);
}
function doOneCat(cat) {
  let promise = doSomeAsyncStuffWithACat(cat);
  promise.catch((err)=> {
    console.error("there was a problem with "+cat+" but processing should continue as normal for the other cats and dogs");
  })
  return promise; 
}

which is working fine when all Cats and Dogs succeed. However, sometimes a cat will fail, and when that happens, I get an exception at the topmost level. The code that deals with a cat failing is inside doOneCat and that is performing correctly. However, the failed promise is still in the promiseArray and so I get a premature "finished".

What is the simplest/canonical method of preventing Promise.all rejecting on the first exception?

pinoyyid
  • 21,499
  • 14
  • 64
  • 115
  • You can check this answer that may help you : https://stackoverflow.com/a/30378082/4409060 – Caribouflex May 16 '19 at 19:35
  • But why? The whole point behind Promise.all() is only move along when all have resolved. If any reject, they all reject. That's the design. – Randy Casburn May 16 '19 at 19:39
  • @randy well it depends. There are legit usecases for both cases (and both can easily be solved using `Promise.all`). (e.g. batching unrelated database operations, if one fails the others still can go on). – Jonas Wilms May 16 '19 at 19:40
  • @Jonas - it is a codesmell and misuse of `Promise.all()` - https://www.ecma-international.org/ecma-262/6.0/#sec-promise.all – Randy Casburn May 16 '19 at 19:42
  • @RandyCasburn if Promise.all is the wrong tool with which to await the completion (regardless of outcome) of an array of async functions, then I'm happy to hear your suggestion for an alternative approach. – pinoyyid May 16 '19 at 19:45
  • @randy who said that? You just quoted the spec which says how Promise.all behaves, not how it should be used. – Jonas Wilms May 16 '19 at 19:45
  • 1
    @RandyCasburn Is there a better native alternative? I'm not a promise expert, but I'm not sure I agree that it's bad semantics: if you're passing a Promise from `.catch` and that `.catch` resolver doesn't raise an error, then the Promise provided to `.all` has the intent that an error from its upstream Promise does not equate to an error in the context of `.all` – apsillers May 16 '19 at 19:46
  • @apsillers et.al - Please see the Userland Implementations section of this TC39 Proposal for `allSettled`. https://github.com/tc39/proposal-promise-allSettled – Randy Casburn May 16 '19 at 19:52
  • @RandyCasburn Ah, I see your point -- surely `allSettled` is indeed best semantics. `:)` There surely are cases where it is semantically appropriate that errors in some specific Promise(s) should *not* short-circuit resolve `.all`, but `.allSettled` ensures that all involved Promises are handled in this way, so if that's your intent, it's the best tool. I concede that you're right it is a code smell, i.e., something that may be done legitimately but is likely is an imperfect attempt to express something else. – apsillers May 16 '19 at 20:01
  • @apsillers not even that. Using the `.catch` version below it will behave *exactly* like the `allSettled` one (the proposal actually shows that). – Jonas Wilms May 16 '19 at 20:05
  • 1
    @JonasWilms Right, I said "there surely are cases..." for a case where one Promise involved in a `.all` should have its exceptions short-circuit `.all` while another Promise in the same `.all` collection should *not* have its exception cause a short-circuit. (I'm just pedantically clarifying my even earlier point in light of the new information of `.allSettled`. I think the utility of my point is vanishingly small, anyway. `:)`) – apsillers May 16 '19 at 20:08
  • @apsillers ah okay, now I get your point ... – Jonas Wilms May 16 '19 at 20:09

2 Answers2

1

Just return the catched Promise, so that the rejection does not bubble up to Promise.all (then it won't exit):

 return promise.catch((err) => {
   console.error("there was a problem with "+cat+" but processing should continue as normal for the other cats and dogs");
    return /*some flag that indicates that this one did throw */;
 });

In the future, there will be Promise.allSettled to solve that exact case.

Jonas Wilms
  • 132,000
  • 20
  • 149
  • 151
  • can you expand on 'some flag...'? Your answer is a return with no args. To clarify, I have no interest in whether each cat was successful or not, only in that all cats have been processed. – pinoyyid May 16 '19 at 19:43
  • @pinoyyid that depends on your usecase. Maybe `null` ? maybe a `Symbol()` ? It depends on the way you want to handle the error. – Jonas Wilms May 16 '19 at 19:47
  • I don't want to handle the error at all. My catch code within `doOneCat(cat)` has already done everything that needs to be done. – pinoyyid May 16 '19 at 19:49
  • 1
    @pinoyyid If you don't use the array of results from `Promise.all(...).then(...)`, then you don't care about the return value at all, and you can omit `return` entirely. If you do use the result of `.all`, then `return` whatever you wish to see in that array. – apsillers May 16 '19 at 19:52
0

I believe these are the available options:-

  1. The answer from Jonas. I tried to implement this, but without success. This may be to my faulty implementation, since my doOneCat() is several layers of nested methods and a nested Promise.

  2. Use a library such as those in https://github.com/tc39/proposal-promise-allSettled linked by Randy.

  3. Refactor to avoid reject/catch altogether as advocated in https://blog.logrocket.com/elegant-error-handling-with-the-javascript-either-monad-76c7ae4924a1 and others.

I eventually went with option 3. I refactored my low level code to resolve with a response object containing a success/fail flag, and my higher level code tests the flag to perform the appropriate success/fail processing. This way my Promise.all only ever sees successfully resolved Promises. imho, this refactoring results in more readable code than the rej/catch code I had before.

pinoyyid
  • 21,499
  • 14
  • 64
  • 115