This is not GitHub.com

Like what you see? Install OctoLinker now!

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test cases for multiple mapper errors with stop on error #49

Merged
merged 7 commits into from Oct 3, 2021

Conversation

@huntharo
Copy link
Contributor

@huntharo huntharo commented Sep 7, 2021

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

@sindresorhus sindresorhus commented Sep 8, 2021

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

test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Sep 8, 2021

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
Copy link
Contributor Author

@huntharo huntharo commented Sep 8, 2021

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
Copy link
Owner

@sindresorhus sindresorhus commented Sep 9, 2021

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
Copy link
Contributor Author

@huntharo huntharo commented Sep 10, 2021

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
Copy link
Owner

@sindresorhus sindresorhus commented Sep 11, 2021

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: #51

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Sep 11, 2021

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.

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
Copy link
Contributor Author

@huntharo huntharo commented Sep 11, 2021

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
Copy link
Owner

@sindresorhus sindresorhus commented Sep 11, 2021

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

{stopOnError: false}?

@huntharo
Copy link
Contributor Author

@huntharo huntharo commented Sep 11, 2021

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
Copy link
Owner

@sindresorhus sindresorhus commented Sep 12, 2021

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.

@huntharo huntharo force-pushed the fix/mapper-exceptions branch from 52abbee to 23c2cd8 Sep 12, 2021
@huntharo huntharo changed the title Add (failing) test case for multiple mapper errors with stop on error Add test cases for multiple mapper errors with stop on error Sep 12, 2021
@huntharo
Copy link
Contributor Author

@huntharo huntharo commented Sep 16, 2021

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?

readme.md Outdated
@@ -70,6 +70,8 @@ Number of concurrently pending promises returned by `mapper`.
Type: `boolean`\
Default: `true`

When set to `true`, the first mapper rejection will be rejected back to the consumer. Caveat: any already-started async mappers will continue to run until they resolve or reject but they cannot be awaited as their promises are not exposed; in the case of infinite concurrency with sync iterables *all* mappers will be started on startup and will continue after a rejection. [Issue #51](issues/51) can be implemented to address additional exception and abort use cases.
Copy link
Owner

@sindresorhus sindresorhus Sep 23, 2021

This is a bit verbose.

And you also need to update index.d.ts.

Copy link
Contributor Author

@huntharo huntharo Oct 2, 2021

done

readme.md Outdated Show resolved Hide resolved
test.js Outdated
}),
{message: 'Oops! 1'}
);
// Note: all 3 mappers get invoked, all 3 throw, even with stop on error this
Copy link
Owner

@sindresorhus sindresorhus Sep 23, 2021

Suggested change
// Note: all 3 mappers get invoked, all 3 throw, even with stop on error this
// Note: All 3 mappers get invoked, all 3 throw, even with `{stopOnError: true}` this

Copy link
Contributor Author

@huntharo huntharo Oct 2, 2021

done

huntharo added 6 commits Oct 2, 2021
- Add a failing test case to indicate that only 1 of the mapper errors is catchable when stop on error is true and many mappers throw an exception
- 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?
- Note: the doc note is now ~25% shorter
@huntharo huntharo force-pushed the fix/mapper-exceptions branch from 5792f9b to 3cb5885 Oct 2, 2021
@sindresorhus sindresorhus merged commit af84dee into sindresorhus:main Oct 3, 2021
3 checks passed
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Oct 3, 2021

Thanks :)

sindresorhus added a commit that referenced this issue Oct 3, 2021
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants