Add test cases for multiple mapper errors with stop on error

[huntharo]

• Add test cases that demonstrate a single exception being rejected when all mappers fail and stop on error is set

[sindresorhus]

When stopOnError: true, it's correct that only the first error is catchable, however, the other errors should be silently caught internally.

[sindresorhus]

With concurrent mappers it seems that exceptions should throw an AggregateError when stop on error is true and more than 1 concurrent mapper throws - the iteration of new input items should stop immediately, but all invoked mappers need to finish and all of their exceptions should be bundled and rejected at once - maybe?

When stopOnError: true, you're indicating that you want to stop on the first error. AggregateError would not be correct in this case.

[huntharo]

With concurrent mappers it seems that exceptions should throw an AggregateError when stop on error is true and more than 1 concurrent mapper throws - the iteration of new input items should stop immediately, but all invoked mappers need to finish and all of their exceptions should be bundled and rejected at once - maybe?

When stopOnError: true, you're indicating that you want to stop on the first error. AggregateError would not be correct in this case.

Right, but the mappers have already been started. Those other mappers will not be canceled when the first error is encountered. They will run to completion. As such, I think the behavior should be: stop dispatching new work, but catch all exceptions for already started items and return them.

I agree that the current implementation does not result in unhandled promise rejections or uncaught exceptions.

I'm not positive how this should work, but it seems wrong that the default behavior will actually start and finish all of the mappers if one of them throws but it only bubbles up 1 random exception even if all of them throw.

In any case, if you decide to keep the current behavior then I can just change the failing test to capture the current behavior to ensure that these behaviors do not accidentally change in the future.

[sindresorhus]

I'm not positive how this should work, but it seems wrong that the default behavior will actually start and finish all of the mappers if one of them throws but it only bubbles up 1 random exception even if all of them throw.

That's how Promise.all works too.

In any case, if you decide to keep the current behavior then I can just change the failing test to capture the current behavior to ensure that these behaviors do not accidentally change in the future.

Yeah, I prefer catching all errors, but only returning the first one.

[huntharo]

I'm not positive how this should work, but it seems wrong that the default behavior will actually start and finish all of the mappers if one of them throws but it only bubbles up 1 random exception even if all of them throw.

That's how Promise.all works too.

Fair point.

I think a distinction here is that with Promise.all the caller has the array of Promises that they can await individually to catch all the exceptions or wait for completion of each item, while with p-map the caller has no way to wait for all created Promises to complete before proceeding as the Promises were not created by the caller and are not exposed to the caller.

A point I'm concerned about here is:

• 1k mappers are started
• 1 mapper fails quickly and throws
• the consumer of p-map has a retry loop
• the retry cannot wait for all the 999 mappers that are still running to complete - thus the retry loop starts before the prior run has finished deterministically
• the consumer ends p with 1,999 mappers running because they have no way to wait until all the previously started mappers are completed before proceeding with their retry

To maintain consistency with Promise.all we'd need to either put a big caveat on stopOnError: true indicating that it really shouldn't be used if you need to wait for any mappers to stop before proceeding after one item throws, OR, we could expose an array of mapper promises for the caller to await or ignore at their own risk. Thoughts on that?

Note that setting stopOnError: false would result in an undesired behavior for any consumer that is NOT using infinite concurrency. With infinite concurrency all of the mappers are started (in theory) before one of them throws, so the throw does not stop more items being enumerated or mapped. With finite concurrency (less than the number of items) an exception stops the invocation of further mappers with stopOnError: true while switching to stopOnError: false would cause all mappers to be invoked even after one of the mappers threw. I think stopOnError: true would be most useful if it only threw after all already-started mappers were completed, but it could be ok for it to throw immediately if the caller at least has a way to wait for all started mappers to finish.

In any case, if you decide to keep the current behavior then I can just change the failing test to capture the current behavior to ensure that these behaviors do not accidentally change in the future.

Yeah, I prefer catching all errors, but only returning the first one.

Ok, let me know what you think of either a doc update with a caveat or exposing an array of mappers that are running when throwing due to stopOnError: true.

[sindresorhus]

Your issue seems to be more about async/await & promises not having a good cancellation story though, but that's a flaw in the language, not just this package. I do want to support AbortController at some point, while far from ideal, get's us closer: https://github.com/sindresorhus/p-map/issues/51

[sindresorhus]

To maintain consistency with Promise.all we'd need to either put a big caveat on stopOnError: true indicating that it really shouldn't be used if you need to wait for any mappers to stop before proceeding after one item throws, OR, we could expose an array of mapper promises for the caller to await or ignore at their own risk. Thoughts on that?

I think the correct approach is to do https://github.com/sindresorhus/p-map/issues/51 and recommend supporting cancellation in the async tasks they do in the mapper.

I think stopOnError: true would be most useful if it only threw after all already-started mappers were completed

IMHO, that would be very unexpected behavior and could cause so many other problems. Errors should not be delayed by default. For example, what if one of the mappers it has to wait on takes a very long time to complete, and in the meantime, some other error is thrown somewhere else in the app, and this error is then forever hidden.

[huntharo]

To maintain consistency with Promise.all we'd need to either put a big caveat on stopOnError: true indicating that it really shouldn't be used if you need to wait for any mappers to stop before proceeding after one item throws, OR, we could expose an array of mapper promises for the caller to await or ignore at their own risk. Thoughts on that?

I think the correct approach is to do #51 and recommend supporting cancellation in the async tasks they do in the mapper.

Supporting cancellation of the mapper tasks would be helpful here, but it still wouldn't allow deterministically waiting until all of the mappers have completed or exited, right? So a programmer still could not write code that said "what for that failure to completely stop, then do this".

Perhaps we need to borrow from p-queue and have an .isIdle() function that returns a promise that has the behavior I'm looking for: all mapper promises have completed (either from cancelling or from completing).

• In the nominal case, with stopOnError: true, with no errors the pMap() promise would resolve and .isIdle() would also immediately resolve if called afterwards
• In the case with an error on the 1st mapper, with stopOnError: true, within 1 second and other mappers run for 30 more minutes
  • pMap() would reject in 1 second, and .isIdle() would resolve in 30 minutes if the mappers did not support cancellation
  • .isIdle() would resolve in less than 30 minutes if the mappers did support cancellation
• When stopOnError: false, .isIdle() could throw since the behavior would be overlapping with the pMap() promise
• It could be implemented so that .isIdle() would reject with an AggregateError of all exceptions that happened other than the first one that caused the pMap() rejection

[sindresorhus]

but it still wouldn't allow deterministically waiting until all of the mappers have completed or exited

{stopOnError: false}?

[huntharo]

but it still wouldn't allow deterministically waiting until all of the mappers have completed or exited

{stopOnError: false}?

Yes that works. Would you accept a documentation caveat being added to the stopOnError docs to point out that already-started mappers will continue to run after the first error is returned and that with the default infinite concurrency that essentially all mappers for the entire collection will have already been started when the first error is thrown?

[sindresorhus]

Yes that works. Would you accept a documentation caveat being added to the stopOnError docs to point out that already-started mappers will continue to run after the first error is returned and that with the default infinite concurrency that essentially all mappers for the entire collection will have already been started when the first error is thrown?

Sure, but try to keep it succinct. It would be nice if it also mentioned https://github.com/sindresorhus/p-map/issues/51, so people would be incentivized to contribute towards that feature.

[huntharo]

Yes that works. Would you accept a documentation caveat being added to the stopOnError docs to point out that already-started mappers will continue to run after the first error is returned and that with the default infinite concurrency that essentially all mappers for the entire collection will have already been started when the first error is thrown?

Sure, but try to keep it succinct. It would be nice if it also mentioned #51, so people would be incentivized to contribute towards that feature.

I added a doc note... I think this one is ready?

[sindresorhus]

Thanks :)