From d16067288d928bacbb85074a453502c497cfd4e8 Mon Sep 17 00:00:00 2001 From: harold Date: Tue, 7 Sep 2021 12:52:22 -0400 Subject: [PATCH 1/4] Add test cases for source iterator exceptions - Source iterator next() functions can throw - There are 3 distinct cases where next() is called, one of which does not have a try/catch around it that will reject the outer promise, resulting in an unhandled promise rejection --- test.js | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/test.js b/test.js index d38ca39..ccd2bc5 100644 --- a/test.js +++ b/test.js @@ -40,6 +40,30 @@ const mapper = async ([value, ms]) => { return value; }; +class ThrowingIterator { + constructor(max, throwOnIndex) { + this._max = max; + this._throwOnIndex = throwOnIndex; + } + + [Symbol.iterator]() { + let index = 0; + const max = this._max; + const throwOnIndex = this._throwOnIndex; + return { + next() { + if (index === throwOnIndex) { + throw new Error(`throwing on index ${index}`); + } + + const item = {value: index, done: index === max}; + index++; + return item; + } + }; + } +} + test('main', async t => { const end = timeSpan(); t.deepEqual(await pMap(sharedInput, mapper), [10, 20, 30]); @@ -128,8 +152,62 @@ test('do not run mapping after stop-on-error happened', async t => { await delay(100); throw new Error('Oops!'); } - }) + }, + {concurrency: 1}) ); await delay(500); - t.deepEqual(mappedValues, [1, 3]); + t.deepEqual(mappedValues, [1]); +}); + +test('catches exception from source iterator - 1st item', async t => { + const input = new ThrowingIterator(100, 0); + const mappedValues = []; + await t.throwsAsync(pMap( + input, + async value => { + mappedValues.push(value); + await delay(100); + return value; + }, + {concurrency: 1} + )); + await delay(300); + t.deepEqual(mappedValues, []); +}); + +// The 2nd iterable item throwing is distinct from the 1st when concurrency is 1 because +// it means that the source next() is invoked from next() and not from +// the constructor +test('catches exception from source iterator - 2nd item', async t => { + const input = new ThrowingIterator(100, 1); + const mappedValues = []; + await t.throwsAsync(pMap( + input, + async value => { + mappedValues.push(value); + await delay(100); + return value; + }, + {concurrency: 1} + )); + await delay(300); + t.deepEqual(mappedValues, [0]); +}); + +// The 2nd iterable item throwing after a 1st item mapper exception, with stopOnError false, +// is distinct from other cases because our next() is called from a catch block +test('catches exception from source iterator - 2nd item after 1st item mapper throw', async t => { + const input = new ThrowingIterator(100, 1); + const mappedValues = []; + await t.throwsAsync(pMap( + input, + async value => { + mappedValues.push(value); + await delay(100); + throw new Error('mapper threw error'); + }, + {concurrency: 1, stopOnError: false} + )); + await delay(300); + t.deepEqual(mappedValues, [0]); }); From bc5f1c08e2146535f0e84f317cbbb674a59c5e84 Mon Sep 17 00:00:00 2001 From: harold Date: Sat, 11 Sep 2021 12:51:19 -0400 Subject: [PATCH 2/4] Catch iteratable.next() exceptions and reject promise --- index.js | 30 +++++++++++++++++++++++++++--- test.js | 31 +++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/index.js b/index.js index 05c8165..933f004 100644 --- a/index.js +++ b/index.js @@ -79,16 +79,40 @@ export default async function pMap( } else { errors.push(error); resolvingCount--; - next(); + + // In that case we can't really continue regardless of stopOnError state + // since an iterable is likely to continue throwing after it throws once. + // If we continue calling next() indefinitely we will likely end up + // in an infinite loop of failed iteration. + try { + next(); + } catch (error) { + if (!isRejected) { + isRejected = true; + reject(error); + } + } } } })(); }; for (let index = 0; index < concurrency; index++) { - next(); + // Catch errors from the iterable.next() call + // In that case we can't really continue regardless of stopOnError state + // since an iterable is likely to continue throwing after it throws once. + // If we continue calling next() indefinitely we will likely end up + // in an infinite loop of failed iteration. + try { + next(); + } catch (error) { + if (!isRejected) { + isRejected = true; + reject(error); + } + } - if (isIterableDone) { + if (isIterableDone || isRejected) { break; } } diff --git a/test.js b/test.js index ccd2bc5..2c72af8 100644 --- a/test.js +++ b/test.js @@ -44,21 +44,27 @@ class ThrowingIterator { constructor(max, throwOnIndex) { this._max = max; this._throwOnIndex = throwOnIndex; + this.index = 0; } [Symbol.iterator]() { let index = 0; const max = this._max; const throwOnIndex = this._throwOnIndex; + const obj = this; return { next() { - if (index === throwOnIndex) { - throw new Error(`throwing on index ${index}`); + try { + if (index === throwOnIndex) { + throw new Error(`throwing on index ${index}`); + } + + const item = {value: index, done: index === max}; + return item; + } finally { + index++; + obj.index = index; } - - const item = {value: index, done: index === max}; - index++; - return item; } }; } @@ -162,15 +168,17 @@ test('do not run mapping after stop-on-error happened', async t => { test('catches exception from source iterator - 1st item', async t => { const input = new ThrowingIterator(100, 0); const mappedValues = []; - await t.throwsAsync(pMap( + const error = await t.throwsAsync(pMap( input, async value => { mappedValues.push(value); await delay(100); return value; }, - {concurrency: 1} + {concurrency: 1, stopOnError: true} )); + t.is(error.message, 'throwing on index 0'); + t.is(input.index, 1); await delay(300); t.deepEqual(mappedValues, []); }); @@ -188,9 +196,10 @@ test('catches exception from source iterator - 2nd item', async t => { await delay(100); return value; }, - {concurrency: 1} + {concurrency: 1, stopOnError: true} )); await delay(300); + t.is(input.index, 2); t.deepEqual(mappedValues, [0]); }); @@ -199,7 +208,7 @@ test('catches exception from source iterator - 2nd item', async t => { test('catches exception from source iterator - 2nd item after 1st item mapper throw', async t => { const input = new ThrowingIterator(100, 1); const mappedValues = []; - await t.throwsAsync(pMap( + const error = await t.throwsAsync(pMap( input, async value => { mappedValues.push(value); @@ -209,5 +218,7 @@ test('catches exception from source iterator - 2nd item after 1st item mapper th {concurrency: 1, stopOnError: false} )); await delay(300); + t.is(error.message, 'throwing on index 1'); + t.is(input.index, 2); t.deepEqual(mappedValues, [0]); }); From 863f2b75529ed92fbbcf2f774654dd220778914d Mon Sep 17 00:00:00 2001 From: harold Date: Sat, 11 Sep 2021 13:02:00 -0400 Subject: [PATCH 3/4] Fix build error and change requests --- index.js | 7 +++---- test.js | 11 +++++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index 933f004..0f8ee62 100644 --- a/index.js +++ b/index.js @@ -106,10 +106,9 @@ export default async function pMap( try { next(); } catch (error) { - if (!isRejected) { - isRejected = true; - reject(error); - } + isRejected = true; + reject(error); + break; } if (isIterableDone || isRejected) { diff --git a/test.js b/test.js index 2c72af8..fdcb7c5 100644 --- a/test.js +++ b/test.js @@ -45,15 +45,15 @@ class ThrowingIterator { this._max = max; this._throwOnIndex = throwOnIndex; this.index = 0; + this[Symbol.iterator] = this[Symbol.iterator].bind(this); } [Symbol.iterator]() { let index = 0; const max = this._max; const throwOnIndex = this._throwOnIndex; - const obj = this; return { - next() { + next: (() => { try { if (index === throwOnIndex) { throw new Error(`throwing on index ${index}`); @@ -63,9 +63,12 @@ class ThrowingIterator { return item; } finally { index++; - obj.index = index; + this.index = index; } - } + // eslint is wrong - bind is needed else the next() call cannot update + // this.index, which we need to track how many times the iterator was called + // eslint-disable-next-line no-extra-bind + }).bind(this) }; } } From bf4c25b59e23352377fd8c26a3cbaeccff33a8fd Mon Sep 17 00:00:00 2001 From: harold Date: Sat, 11 Sep 2021 13:24:34 -0400 Subject: [PATCH 4/4] Fix change request --- index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 0f8ee62..85d8599 100644 --- a/index.js +++ b/index.js @@ -87,10 +87,8 @@ export default async function pMap( try { next(); } catch (error) { - if (!isRejected) { - isRejected = true; - reject(error); - } + isRejected = true; + reject(error); } } }