sindresorhus / p-map

Map over promises concurrently
MIT License
1.27k stars 58 forks source link

Support aborting the mapping #28

Closed niftylettuce closed 3 years ago

niftylettuce commented 4 years ago

e.g. a few seconds into waiting for the promise to finish, we could increase concurrency due to CPU limit being lowered below 20%, to a concurrency of 5x or something. Basically dynamic concurrency

niftylettuce commented 4 years ago

Perhaps expose or pass a Symbol?

sindresorhus commented 4 years ago

Where would you change the concurrency? Inside the map function?

sindresorhus commented 4 years ago

Perhaps expose or pass a Symbol?

Can you show an example of how that would be used?

niftylettuce commented 4 years ago

Here's an example:

https://github.com/breejs/bree#nodejs-email-queue-job-scheduling-example

Regarding the Symbol, I'm not sure to be honest because there would need to be support for multiple instances somehow.

niftylettuce commented 4 years ago

In the prior link shared, I am referencing this snippet:

// handle cancellation (this is a very simple example)
if (parentPort)
  parentPort.once('message', message => {
    //
    // TODO: once we can manipulate concurrency option to p-map
    // we could make it `Number.MAX_VALUE` here to speed cancellation up
    // <https://github.com/sindresorhus/p-map/issues/28>
    //
    if (message === 'cancel') isCancelled = true;
  });
sindresorhus commented 4 years ago

While I agree that the ability to dynamically change the concurrency could be useful, in the example you reference, I think what you actually want is the ability to abort the p-map call. Could maybe be something like pMap.abort(mapper).

niftylettuce commented 4 years ago

Yes that would be great!

niftylettuce commented 4 years ago

Both would, but the latter you mention would solve the immediate issue.

papb commented 4 years ago

@sindresorhus Interesting idea of using the mapper reference itself to abort, but wouldn't it be better to add an .abort method to the returned promise? Otherwise if someone calls pMap twice with the same mapper they might get an undesired behavior.

- await pMap(foo, bar);
+ const mapping = pMap(foo, bar);
+ setTimeout(mapping.abort, 5000);
+ await mapping;
sindresorhus commented 3 years ago

Otherwise if someone calls pMap twice with the same mapper they might get an undesired behavior.

Yeah, good point.

Maybe supporting AbortController would be an even better solution.

sindresorhus commented 3 years ago

Duplicate of #31