-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Unoptimized implementation of pMapIterable
preserveOrder
option
#79
base: main
Are you sure you want to change the base?
Unoptimized implementation of pMapIterable
preserveOrder
option
#79
Conversation
…fer and `promiseEmitter` - move `promise` inner `try-catch` logic to `promise.catch` (now also catches when `iterator.next()` throws) - when `returnValue === pMapSkip`, `popRandomPromise` unconditionally (skip `popNextPromise` in main `while` loop instead) - add tests for `preserveOrder: false`, sync throwing mappers/iterables, more intricate `pMapSkip` scenarios
} | ||
|
||
trySpawn(); | ||
|
||
while (promises.length > 0) { | ||
const {error, done, value} = await promises[0]; // eslint-disable-line no-await-in-loop | ||
const {promise, result: {error, done, value}} = await nextPromise(); // eslint-disable-line no-await-in-loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline the function here
|
||
promises.push(promise); | ||
promise.then(p => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use try/catch/async/await here. Also, would be better to use a deferred promise and we call it after anything resolves, instead of racing? https://github.com/sindresorhus/p-defer
// This occurs when `concurrency > 1`: the first `promise` will `trySpawn` and `promises.push` another promise before `await`ing the mapper, | ||
// but the ongoing `Promise.race(promises)` call from `nextPromise` is oblivious to this new promise as it was not present in `promises` | ||
// when the race began. | ||
new Promise(resolve => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good thinking!
Fixes #72.
Spun off from #74 in light of
pMapIterable
preserveOrder
option #74 (comment)pMapIterable
preserveOrder
option #74 (comment)Summary
Adds a
preserveOrder
option topMapIterable
, which indicatesImplementation considerations
promise
must return reference to itself sopopRandomPromise
can findindexOf
.preserveOrder: false
, racing thepromises
pool is not enough, sincetrySpawn
may be add additional promises to the buffer after the race has begun: so we race an event listener alongside (promises
pool is still needed to buffer multiple promises finishing simultaneously.returnValue === pMapSkip && preserveOrder: false
, unlike whenpreserveOrder: false
we have no way to know if returning will cause ourpromise
to win an ongoing race, and so have no way of knowing whether to skip popping ourselves (to allow the main loop to pop us instead). So, whenreturnValue === pMapSkip
, we insteadpopRandomPromise
ourselves unconditionally and instead skippopNextPromise
in mainwhile
loop.promise
innertry-catch
logic topromise.catch
(which now also catches wheniterator.next()
throws)preserveOrder: false && result.done
, some promises in the pool may still be pending, socontinue
(as a matter of fact, we could probablycontinue
in all cases since the current implementation does not run concurrentiterator.next
calls, so ifpreserveOrder: true && result.done
there should be no remainingpromises
in the queue - but hopefully that will change soon as we think about concurrentiterator.next
calls!