Skip to content

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Sep 28, 2023

What's the problem this PR addresses?

This PR extends @anuragkalia's work in #5636 (it starts from the same commits and builds upon them).

  • The original implementation was keeping a singleton worker pool - I felt this was problematic since we may enter situations (even if only in tests) where multiple calls to convertToZip would want to use different pools.

  • Converting an archive without using a worker was only possible when the concurrency was completely disabled. This was my idea, but retrospectively it felt better to me to have two settings: one for the concurrency, and another to define whether the concurrency is enabled or not.

  • Passing all those settings from the various fetchers would have been unwieldly.

How did you fix it?

  • I created a TaskPool interface; WorkerPool implements it (using workers, same implementation as before), but so does AsyncPool (a new implementation that simply forwards the call to the pLimit limiter).

    • I feel like this also addresses @merceyz's comment regarding the setting name - it's now called taskPoolConcurrency, which doesn't directly refers to workers.
  • A getConfigurationWorker function returns the proper task pool for the given configuration object. To make that possible, WeakMap instances can now be used as storage argument for the get*WithDefault family of functions.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis arcanis merged commit fdfc784 into master Oct 2, 2023
@arcanis arcanis deleted the mael/task-pool branch October 2, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants