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

Cache downloads individually upon completion #2756

Merged

Conversation

HebaruSan
Copy link
Member

Problem

If you select many uncached mods to install, they're all downloaded to temp files in your temp folder before being moved to the CKAN download cache. The temp files remain until all of the downloads fail or finish. This consumes disk space in the temp folder for longer than strictly necessary and delays the insertion of individual files into the cache.

image

Cause

NetAsyncModulesDownloader and NetAsyncDownloader communicate via a callback function that's only called at the end of the download process. So NetAsyncModulesDownloader waits for all downloads to complete before ever calling cache.Store.

Changes

  • NetAsyncDownloader.onCompleted is replaced by onOneCompleted, which is called once per individual download, when that download fails or finishes
  • NetAsyncModulesDownloader.ModuleDownloadsComplete is replaced by ModuleDownloadComplete which handles one download at a time
  • The cache parameter of NetAsyncModuleDownloader.DownloadModules is moved to that class's constructor, and all calling code is updated to accommodate this (the motivation is to be able to use ModuleDownloadComplete cleanly as the callback without a lambda)

Note that this change is not guaranteed to fix any real world problem. If you were having a problem with your temp folder filling up before, that exact problem can still happen if the timing of the downloads works out such that they all last a long time. You will always need enough temp folder space available to handle all downloads that you initiate.

Fixes #2746.
Fixes #1954 (probably fixed a long time ago, but now we can say that the possibility of this happening has been eliminated).

@HebaruSan HebaruSan added Enhancement New features or functionality Pull request Network Issues affecting internet connections of CKAN labels May 8, 2019
@Olympic1 Olympic1 removed Enhancement New features or functionality Network Issues affecting internet connections of CKAN labels May 8, 2019
@HebaruSan HebaruSan added Enhancement New features or functionality Network Issues affecting internet connections of CKAN Core (ckan.dll) Issues affecting the core part of CKAN labels May 8, 2019
@DasSkelett
Copy link
Member

DasSkelett commented May 8, 2019

Note that this change is not guaranteed to fix any real world problem. If you were having a problem with your temp folder filling up before, that exact problem can still happen if the timing of the downloads works out such that they all last a long time. You will always need enough temp folder space available to handle all downloads that you initiate.

We already have the download size of the mods in the metadata, and the .tmps are just the "unnamed" zips, right? Shouldn't it be possible to display a warning before the download starts if all mods together are bigger than the remaining disk space in the temp folder?

@HebaruSan
Copy link
Member Author

Shouldn't it be possible to add a warning before the download starts if all mods together are bigger than the remaining disk space in the temp folder?

Yes... if .NET provided a cross platform "free space in folder" function. The only one I've been able to find is DriveInfo.AvailableFreeSpace, which I think is only useful on Windows.

@DasSkelett
Copy link
Member

which I think is only useful on Windows.

Why? Is it buggy/not working/not implemented on Mono?

@HebaruSan
Copy link
Member Author

Other OSes don't have a concept of a "drive" in the same way Windows has "C:" and "D:", and I don't think the runtime provides a cross-platform way to translate from a folder path to a Drive. That was from the last time I looked into it, though, so I'm prepared to be proven wrong.

@DasSkelett
Copy link
Member

DasSkelett commented May 8, 2019

Yeah, I tested it, and it seems that mono can't get a drive info for the current directory, if it's not exactly point where the according drive/partition is mounted (that means it works for /media/... or root / itself, but not f.e. for /home/user/documents/ when the home folder is a symlink to another drive.)
And Path.GetPathRoot() always returns / and I found no way to find the mount point of the drive for the cwd.

I guess you're right :/

@HebaruSan
Copy link
Member Author

Yeah, I think this last came up when we were consolidating the cache folder. The only reliable way to detect disk overflows is after the fact, with exceptions.

@DasSkelett
Copy link
Member

Pffft, now I did spot where to dismiss reviews again...
Well, for the next time 😄

@HebaruSan HebaruSan merged commit 6cb7dbe into KSP-CKAN:master May 9, 2019
@Olympic1 Olympic1 removed Enhancement New features or functionality Network Issues affecting internet connections of CKAN Pull request labels May 9, 2019
@HebaruSan HebaruSan deleted the feature/instant-download-store branch May 9, 2019 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add downloads to cache immediately upon individual completion Don't fail all updates if one download fails
3 participants