Concurrent downloads & installation #4570
Conversation
|
It compiles, but I believe there is an infinite loop; at least, I could not get any toolchain installation to finish as it seemed to hang on the download of the last component. Additionally, I would like to note that when a component is being installed, although ongoing downloads continue normally, new component downloads cannot start. This isn’t an issue when Finally, I would like to point out that running both downloads and installations on the same thread prevents the progress bars for other downloads from updating (see here). |
| let mut downloads = FuturesUnordered::new(); | ||
| let mut component_iter = components.iter(); | ||
| let mut cleanup_downloads = vec![]; | ||
| loop { |
There was a problem hiding this comment.
@djc Thanks for the testbed!
After careful comparisons, I think this is equivalent to b5e9e4d (#4471) once we squash 5800cba (#4471) back onto it to eliminate the use of channels, modulo some edge cases such as empty todo list and download failures.
The core of this approach includes directly issuing a blocking call to what we have pulled from a FuturesUnordered. When that blocking call is running, there will be no progress updates and no new installations being initiated. So this is the same as what we've had after the very first step of #4471.
There was a problem hiding this comment.
Thanks for looking into. Do you think it's useful to merge (something like) this and proceed from there?
When that blocking call is running, there will be no progress updates and no new installations being initiated.
If this is the main issue that's left, it feels like there might be easier (and better) ways to solve this.
There was a problem hiding this comment.
@djc I think that is THE remaining issue, yes.
However I still have concerns regarding the aforementioned edge cases that I and @FranciscoTGouveia have found out when working on #4471 so more careful checks have to be carried out.
I think it'd be nice if both of us can have another look at this PR to ensure the semantics are right, it might take some time though while I try to coordinate 🙇
There was a problem hiding this comment.
However I still have concerns regarding the aforementioned edge cases that I and @FranciscoTGouveia have found out when working on #4471 so more careful checks have to be carried out.
I think it'd be nice if both of us can have another look at this PR to ensure the semantics are right, it might take some time though while I try to coordinate 🙇
Sounds great!
|
I've fixed this up so that it can at least pass basic manual tests. CI is failing because I changed the log output -- happy to fix this up if we decide that we want to move forward with this. |
This is what I had in mind. It seems to compile but I've done no testing or benchmarking.
@FranciscoTGouveia maybe that's something you can have a look at?
Alternative to
Obsoletes