Homebrew / brew

🍺 The missing package manager for macOS (or Linux)
https://brew.sh
BSD 2-Clause "Simplified" License
41.66k stars 9.79k forks source link

Tracking Issue: Concurrent Downloads #18278

Open reitermarkus opened 2 months ago

reitermarkus commented 2 months ago

Concurrent Downloads

MikeMcQuaid commented 2 months ago

Thanks for the write-up @reitermarkus! Makes sense to me. Please focus on getting https://github.com/Homebrew/brew/pull/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 commented 2 months ago

Thoughts so far:

reitermarkus commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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.