Skip to content
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

Tracking Issue: Concurrent Downloads #18278

Open
1 of 7 tasks
reitermarkus opened this issue Sep 7, 2024 · 6 comments
Open
1 of 7 tasks

Tracking Issue: Concurrent Downloads #18278

reitermarkus opened this issue Sep 7, 2024 · 6 comments
Labels
features New features help wanted We want help addressing this

Comments

@reitermarkus
Copy link
Member

Concurrent Downloads

  • Implement MVP of concurrent downloads for brew fetch.

    Implemented in Implement concurrent downloads in brew fetch. #17756.

  • Improve output logic.

    For simplicity, the output currently only uses at most terminal height - 1 lines due to a trailing newline.

  • Deprecate custom download strategies.

    In order to implement the following parts, custom download strategies need to be deprecated. Their API surface is too big since they depend on many private methods from their superclass, making changes here practically impossible without breaking things.

  • Implement progress bar.

    Every download strategy needs to support progress reporting.

  • Implement graceful cancellation of downloads.

    Currently, cancelling downloads can only be done by killing the whole thread pool, i.e. the sledgehammer approach. Proper cancellation based on https://ruby-concurrency.github.io/concurrent-ruby/master/Concurrent/Cancellation.html should be implemented, making it possible to neatly show successful, failed and cancelled downloads.

  • Implement concurrent downloads for brew install, brew reinstall etc.

  • Enable concurrent downloads by default.

@reitermarkus reitermarkus added the features New features label Sep 7, 2024
@MikeMcQuaid MikeMcQuaid added the help wanted We want help addressing this label Sep 7, 2024
@MikeMcQuaid
Copy link
Member

Thanks for the write-up @reitermarkus! Makes sense to me. Please focus on getting #17756 merged ASAP.

  • In order to implement the following parts, custom download strategies need to be deprecated. Their API surface is too big since they depend on many private methods from their superclass, making changes here practically impossible without breaking things.

We can't/won't do this. Things as basic as "download a release from a private GitHub repository" require this. Instead we should expect to provide suboptimal/poor/no progress reporting, graceful cancellation for these strategies.

@MikeMcQuaid
Copy link
Member

Thoughts so far:

  • missing support for falling back to earlier bottle tags
$ brew fetch --retry --concurrency=10 boost qt
Fetching: boost, qt
Warning: Bottle for tag :arm64_sequoia is unavailable.
Warning: Bottle for tag :arm64_sequoia is unavailable.
  • the above message should probably also warn which bottle it relates to
  • we should limit the hosts we'll download in parallel from to an allowlist so we don't e.g. try to download the 10 files at once from a poor personal web server when building from source

@reitermarkus
Copy link
Member Author

We can't/won't do this. Things as basic as "download a release from a private GitHub repository" require this.

If it's that basic, we should support it using an official download strategy. In any case, we don't want to maintain two different types of download strategies.

@MikeMcQuaid
Copy link
Member

@reitermarkus We historically supported many more types of download strategies that we didn't use e.g. those for private resources. Problem is: when we don't actually use and rely on them ourselves, they end up bitrotting.

In general: even if we were to support them: I'd rather not break an existing, public (implicit or not) API for people for new functionality if we can just degrade to not support that functionality there instead.

@Bo98
Copy link
Member

Bo98 commented Sep 10, 2024

This will probably be easier to discuss with a demonstration of what exactly needs changing in download strategies that would be a breaking change.

@Bo98
Copy link
Member

Bo98 commented Sep 10, 2024

Concurrent::Cancellation isn't stable yet so isn't actually available in concurrent-ruby. It's a WIP feature in a separate beta gem for now.

Worth noting that download strategies already need to support Ctrl+C Interrupt exceptions I think. So Thread#raise Interrupt shouldn't be surprising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features New features help wanted We want help addressing this
Projects
None yet
Development

No branches or pull requests

3 participants