Move downloads outside of gui transaction #2073
Conversation
|
Cleaning out PR list. |
|
The concept seems sound to me. I don't have a good understanding of that section of the code. |
|
@politas It's been awhile since I looked, but, from memory... Basically, a transaction is begun as soon as install is initiated, and during that transaction (which has a 10 minute maximum timeout) we have to download and install all the mods. This usually isn't a problem, but several things can then cause the transaction to time out: lots of mods, a single mod with a large download, or a single download from a slow-ass server. This patch causes the download process to take place before the transaction is actually started. As noted above, this risks causing changes to the download cache not to be covered by the transaction, but I don't know if the download cache is meant to be covered; I kind of just assumed it's not. |
|
The only issue I can see is an incomplete download getting cached and poisoning further attempts? |
|
@politas So the download cache is transacted? |
|
I believe we add any successful downloads to the cache regardless of the transaction success, and it looks like Core/Net/NetAsyncModulesDownloader method ModuleDownloadsComplete only adds completed downloads to the cache, so taking the downloads out of the transaction should be ok. I note that you tested this successfully; what platform do you use? |
|
That's good to know about the downloads. My system is Windows 10.
…On Thu, Aug 17, 2017 at 2:53 PM Myk ***@***.***> wrote:
Reopened #2073 <#2073>.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2073 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AApeRijLk8k9yvP3bW1Il9Cr6vdPDRchks5sZJoogaJpZM4N5aLL>
.
|
| // | ||
| // JObject obj = JObject.Parse(json); | ||
| // return obj.IsValid(metadata_schema); | ||
| license = license ?? new List<License> { License.UnknownLicense }; |
There was a problem hiding this comment.
If I understand this correctly, this sets licence to "unknown" if it doesn't match anything in the licence list? We would generally prefer a failure (this code is mainly intended for netkan)
There was a problem hiding this comment.
@politas The old code would create a new instance of an unknown license in that case, on line 367. The new code does the same thing but uses the static License.UnknownLicense instead of a new instance. I probably shouldn't have been doing arbitrary little cleanup-y things like that in a branch about timeouts, but the effect (creating the unknown license) is not changed.
There was a problem hiding this comment.
Ok. The little code cleanups are making it hard to review, given my poor understanding of the transaction code!
ea14a4e to
0d38529
Compare
|
@politas I have categorized the work into different commits based on what it actually does. I hope that will make this easier to review. In theory, it should also make it easier to cherry-pick only the parts of the work you want at the moment, if you prefer to go that route. |
| @@ -96,6 +97,31 @@ public partial class Main | |||
| installCanceled = true; | |||
| }; | |||
|
|
|||
| GUI.user.RaiseMessage("About to install...\r\n"); | |||
There was a problem hiding this comment.
Well, that sucks. I'll poke that when I get home and see if I can understand what's going on.
There was a problem hiding this comment.
Whoops, I deleted this comment from the commit in order to move it to the PR:
"This message is appearing when the only actions are removing mods, and the client seems to get stuck after that."
There was a problem hiding this comment.
I haven't tested updates, but I suspect they may have the same problem.
There was a problem hiding this comment.
I haven't made any updates; I just rearranged the pull request to be easier to review. I didn't get to leave my day job yesterday until dark thirty. :(
There was a problem hiding this comment.
Sorry, I meant "I haven't tested running a changeset that only contains updates to installed mods, but I suspect it would suffer the same problem"
This change simplifies the `DeserializationFixes` method. Fom what Visual Studio is telling me right now, that method isn't actually used to begin with, so maybe we'd be better off deleting it--but I'm on a mac right now, so I wouldn't take my word for it.
This class allows us to move the module resolution and download process outside the GUI transaction by permitting us to treat both cached and uncached modules equivalently during the lead up to that process.
0d38529 to
b2cfe5f
Compare
Here we use the new ModuleResolution class to perform the work of downloading uncached modules before initiating the GUI transaction wrapping the install process.
b2cfe5f to
565269c
Compare
|
@politas If you look at the It now just aborts if there's nothing to download. |
|
@archer884 , is there a new commit you need to push up to GitHub for that fix? |
|
@politas No, the change is already in here. What I did is that I took this opportunity to rebase on master, pull in all the latest changes from there, and then update my previous commit with the corrected code. |
|
That seems to have fixed up that issue. Thanks for the reorg, makes it much clearer! |
|
Now we just need to work out why Jenkins is complaining. @techman83, can you have a look if you've got a minute? Might be more @dbent's sphere of expertise, though. |
|
Looks like just a transient issue connecting to the mono repos. I've restarted the build, it'll probably be fine now. |
|
Ah, beautiful. Merging! |
* License cleanup This change simplifies the `DeserializationFixes` method. Fom what Visual Studio is telling me right now, that method isn't actually used to begin with, so maybe we'd be better off deleting it--but I'm on a mac right now, so I wouldn't take my word for it. * Remove unused code * Cleanup usings * Whitespace cleanup * Add ModuleResolution class This class allows us to move the module resolution and download process outside the GUI transaction by permitting us to treat both cached and uncached modules equivalently during the lead up to that process. * Move download outside GUI transaction Here we use the new ModuleResolution class to perform the work of downloading uncached modules before initiating the GUI transaction wrapping the install process. * Merge KSP-CKAN#2073 Move downloads outside of gui transaction
#1935 mentions a timeout exception impacting only the GUI on large and/or slow downloads (taking longer than ten minutes). I believe this can be solved by moving the download process outside of the transaction begun in the GUI (there are additional transactions started inside Core which are not impacted by download times).
Of course, this is a terrible idea if changes to the cache are intended to be covered by the transaction, but I have not dug into the cache code deeply enough to know if this is the case. Thoughts?
To minimize the changes required (and thereby maximize my odds of not breaking anything), I have created the new download workflow in addition to the old one rather than replacing the old one. This is largely because the old one is in use in lots of different places throughout the GUI code--for additions and updates as well as bulk installs. This PR pretty much just overhauls bulk installs, since they exhibited the problem for me when I tried to update for 1.3 the other day.
I have just used a build based on this to update my KSP install, so it works. On my machine. We all know what that's worth, surely. :)
First PR here. If I have goofed on anything, I apologize.