fix(async): pooledMap surfaces errors thrown by the input iterable#7169
fix(async): pooledMap surfaces errors thrown by the input iterable#7169LeSingh1 wants to merge 2 commits into
Conversation
The catch block in pooledMap's writer loop discarded the error that came out of `for await (const item of array)` and built an AggregateError populated solely from the executing transformations. When the iterable itself threw (and no transformation had rejected), that meant an empty `AggregateError.errors` — callers saw only the generic "Cannot complete the mapping" message with no way to recover the underlying cause (denoland#6716). Bind the caught value as `iterError` and include it in `errors` if it isn't already there. Promise.race rejections from the executing pool are already surfaced via the existing allSettled walk, so the `includes` check prevents duplicates for the common transformation- rejection path. Adds a regression test covering the iterable-throws case and verifies the existing 'handles errors' test still produces exactly the expected pair of rejections. Fixes denoland#6716
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7169 +/- ##
=======================================
Coverage 94.57% 94.57%
=======================================
Files 636 636
Lines 52142 52143 +1
Branches 9401 9402 +1
=======================================
+ Hits 49315 49316 +1
Misses 2249 2249
Partials 578 578 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bartlomieju
left a comment
There was a problem hiding this comment.
Solid fix for a real silent-failure bug. The identity-based dedup is correct — in the transformation path Promise.race rejects with the same object that Promise.allSettled later surfaces, so includes suppresses the duplicate and the existing handles errors test still yields exactly two errors. All 6 tests pass locally.
Two small, non-blocking notes inline.
| // throws the original error would otherwise be swallowed, leaving | ||
| // callers with an empty AggregateError (see #6716). Add it only if | ||
| // it isn't already accounted for. | ||
| if (!errors.includes(iterError)) errors.push(iterError); |
There was a problem hiding this comment.
Non-blocking edge case: includes dedups by ===. If the iterable throws a non-object primitive (e.g. a string or number) that happens to equal a transformation rejection reason already in errors, the iterable's error would be silently dropped. It's an extreme corner and the current behavior is reasonable, but worth a one-line comment noting the dedup is identity/value-based so the assumption is explicit.
| const results = pooledMap( | ||
| 2, | ||
| errorThrowing(), | ||
| (i: number) => Promise.resolve(i), |
There was a problem hiding this comment.
This transformation (Promise.resolve(i)) never rejects, so the test only exercises the pure iterable-throw path. Consider adding a combined case where the iterable throws while a transformation is also in flight and rejecting — that would lock in that both the iterable error and the transformation rejections appear together in AggregateError.errors, which is exactly the interaction the !errors.includes(iterError) guard governs. Optional, since this test plus the existing handles errors test cover the two paths individually.
Fixes #6716.
When an async iterable passed to
pooledMapthrows (rather than a transformation), the catch block discarded the error and built anAggregateErrorpopulated solely from the executing transformations. With no transformation rejections in flight, callers saw only:The underlying cause was unrecoverable.
Repro from the issue:
After the patch the iterable's error rides through in
AggregateError.errors:Implementation: bind the catch's value as
iterErroranderrors.push(iterError)only when it isn't already inerrors. ThePromise.racerejections from the executing pool are already surfaced via the existingallSettledwalk, so theincludescheck prevents the common transformation-rejection path from duplicating its own reason.Tests:
pooledMap() surfaces errors thrown by the input iterable (#6716)pins the iterable-throws case.pooledMap() handles errorstest (which exercisesPromise.racerejections, not iterable throws) still produces exactly two rejections, confirming no double-counting.All 6
pool_test.tstests pass.