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

Store downloaded files in a persistent directory until installation #958

Merged
merged 5 commits into from
Mar 22, 2017

Conversation

jelford
Copy link
Contributor

@jelford jelford commented Feb 19, 2017

This PR should go some way to addressing #889 by caching downloaded
components in a persistent directory until they have been installed.

This way, if the download/install process is interrupted, the file
that made it can be re-used. This should help ease the pain of
intermittent connections.

This PR should go some way to addressing rust-lang#889 by caching downloaded
components in a persistent directory until they have been installed.

This way, if the download/install process is interrupted, the file
that made it can be re-used. This should help ease the pain of
intermittent connections
@jelford
Copy link
Contributor Author

jelford commented Feb 19, 2017

I'd like to go further and add support for resuming partial downloads, but I'm not sure what the direction is with e.g. rustup-dist::download. It doesn't look used (although it is public, so not sure...? This is my first proper bit of rust, so not familiar with how these things work yet).

I'd like to refactor the logic I've put into dist::DownloadCfg into a new module (which I would call download, were it not for the presence of an existing module there), and have that encapsulate All Things Download-Related, but could do with a bit of guidance on what's appropriate / the intended direction of the existing stuff before I go too far. I think what's here already should be useful for some people (it would have been enough to help me out on the recent train journey that inspired the work), but if there's interest I'd be happy to follow up with a new PR to finish the job in the future.

Also replaced the usage of download::DownloadCfg with a simple call
to utils::download_file, to simplify the number of names being used
in the file.

The error behaviour when a component fails to download has changed
slightly; underlying errors get wrapped in a ComponentDownloadFailed,
rather than propogating out of manifest::update directly. I think this
should be okay because the error_chain doesn't throw the info away.
@brson
Copy link
Contributor

brson commented Mar 17, 2017

Hi @jelford. This looks excellent so far. Thank you, and sorry for taking so long.

The download code in rustup is a mess. Curl is the backend we use, so for now I'd say just find that code path and tell curl to do the resume, ignore the others.

In the future I'd like to just delete that crate end use the reqwest crate (#993).

Is there any benefit to doing this without the download resumption?

@jelford
Copy link
Contributor Author

jelford commented Mar 17, 2017

Hi @brson, thanks for taking a look, no worries about the wait.

I agree that this would be better with partial-download resumption, but what I'd say is this: even with just support for resuming in the whole-file case, this would have helped me out a lot on the train (and a few people have suggested that as a first step in eg the linked issue) a couple of times now (LTE internet in the English countryside is... not what it could be).

I'll try to find a time to do partial downloads, but can't say how soon (I don't have allocated time for open source), so if you feel you can merge this as it is (or with some not-too-drastic changes), that might be worthwhile.

@brson
Copy link
Contributor

brson commented Mar 17, 2017

resuming in the whole-file case

Aha. I did not think of that for some reason.

@brson
Copy link
Contributor

brson commented Mar 17, 2017

I feel pretty good about this as-is. My main concern is about garbage collection - what happens to old, unresumed downloads? Are they just going to sit on disk forever?

ISTM it might be appropriate to delete the entire download directory after a successful toolchain installation. Seems like the big concern doing that would be about concurrent rustup execution (which is totally broken today anyway).

cc @Diggsey Do you have any opinions about this feature?

@Diggsey
Copy link
Contributor

Diggsey commented Mar 17, 2017

This looks good for me - I'm not too worried about clean-up, the code already seems to clean up files that were used after a successful install, so the only left over files come from failed download attempts which are never successfully retried.

It would be good to get a regression to test on this (just make sure it outputs "reusing downloaded file" in at least one case when expected).

@brson
Copy link
Contributor

brson commented Mar 17, 2017

Oh good point. I agree this needs a test if we can. I'm not sure how to simulate a failed download locally though. That might require adding some instrumentation to rustup, like if RUSTUP_MOCK_DL_FAIL is set, fail.

For a test case, probably crib off one of the simple cases in cli-v2.

@jelford
Copy link
Contributor Author

jelford commented Mar 17, 2017

Thanks, I'll have a look at adding a test-case, but it might have to wait until later next week.

I think the garbage-collection situation should be okay, if one presumes that mostly people would aim to retry failed downloads anyway. It will be more of an issue with partial downloads (here, anything partial can get cleaned, so you can say "if the hash-check fails, smoke it" - whereas in the partial case you can't use the hash). The "delete everything on success" solution would certainly work, if it's a worry.

That AppVeyor failure: I don't have a windows machine, so I might need a hand to look into that.

@brson
Copy link
Contributor

brson commented Mar 17, 2017

Don't worry about AppVeyor. The windows build is flaky at the moment.

The test is only that the file-reuse notification is fired. In order to
detect this in the test, I've pushed through the notification handler
that gets given to `manifestation.update`, which in practice is nearly
always a clone of the one in DownloadCfg (but notably not when called as
in the test cases).
@jelford
Copy link
Contributor Author

jelford commented Mar 20, 2017

Well, it looks like I did actually find some time. I've just pushed some tests around file re-use. I've also opened up a new branch in my repo with some changes to support partial-download resumption (only for curl, as discussed).

I'm not sure your preferred workflow: would you like to get what's here finished off (if you have any more changes you'd like) and merged in, then I'll open a new PR for partials, or would you rather just have the whole lot in one go (I can close this PR and open a new one).

I'd warn that I'm not sure what you'll make of the way I've done the testing for partials (added a local http-server that spins up and server files- actually the code is ripped out of a toy download manager I've been tinkering with), so it would be better not to end up with a bunch of things you don't want to merge getting attached to this one.

@brson
Copy link
Contributor

brson commented Mar 20, 2017

@jelford Let's land this first and do the rest as a followup.

@brson
Copy link
Contributor

brson commented Mar 20, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Mar 20, 2017

📌 Commit 2ce8d72 has been approved by brson

@bors
Copy link
Contributor

bors commented Mar 20, 2017

⌛ Testing commit 2ce8d72 with merge 9ec6d72...

bors added a commit that referenced this pull request Mar 20, 2017
Store downloaded files in a persistent directory until installation

This PR should go some way to addressing #889 by caching downloaded
components in a persistent directory until they have been installed.

This way, if the download/install process is interrupted, the file
that made it can be re-used. This should help ease the pain of
intermittent connections.
@bors
Copy link
Contributor

bors commented Mar 20, 2017

💔 Test failed - status-appveyor

@brson
Copy link
Contributor

brson commented Mar 22, 2017

@bors retry

@bors
Copy link
Contributor

bors commented Mar 22, 2017

⌛ Testing commit 2ce8d72 with merge a1287b6...

bors added a commit that referenced this pull request Mar 22, 2017
Store downloaded files in a persistent directory until installation

This PR should go some way to addressing #889 by caching downloaded
components in a persistent directory until they have been installed.

This way, if the download/install process is interrupted, the file
that made it can be re-used. This should help ease the pain of
intermittent connections.
@bors
Copy link
Contributor

bors commented Mar 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: brson
Pushing a1287b6 to master...

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.

4 participants