Don't revert removals on providing mod choice#4609
Merged
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
341589a to
909324a
Compare
909324a to
13ed64e
Compare
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Cause
Exactly one year ago, #4367 moved GUI's installation transaction from outside the
for (bool resolvedAllProvidedMods = false; ...loop to inside of it.When
InstallListthrowsTooManyModsProvideKraken(as it does many times for Sol-Configs because it has a lot of providing mod choices), thetryblock inside the transaction catches and handles it (so the exception does not directly cause the transaction to be aborted), but we then exit the transaction's scope without finalizing it, which causes the transaction to abort, which restores the installed mods on disk and in the registry.We then try to apply the changeset again, but the uninstallation step is skipped because the
toUninstall.Clear();line has already cleared the list of mods to uninstall after the first uninstallation step. This means that whenInstallListis called, the conflicting mod (ParallaxContinued in the example) is still present on disk and in the registry.The only workaround is to perform the removals separately in an earlier changeset.
The reason given for this change in #4367 was:
This happens with the original transaction scope if the user clicks Retry in the failed downloads dialog:
This happens after the failed downloads kraken is (and must be) thrown within
InstallList's transaction scope.TransactionScopedoes not support nesting in the intuitive sense that I thought it did (i.e., that a sub-transaction could be rolled back independently while its parent transaction continues on with further sub-transactions until they all commit together). Instead, aborting a child transaction aborts the parent, so this exception causes the main outer transaction to abort as well, so we can't use it to retry downloads.(There is a way to get an independent "child" transaction using
TransactionScopeOption.RequiresNew, but these do not revert if the child is completed and the parent is aborted, so that won't work for us.)So we can't just have a loop outside the transaction scope, or we'll revert uninstallations every time there's a provides prompt. And we can't just have a loop inside the transaction scope, or we'll try to retry downloads with an aborted transaction. Hmm. ⚖️
Changes
MainInstall👶🪓: One outside the transaction scope to retry after exceptions that abort the transaction, and another inside the transaction scope to retry after exceptions that do not abort the transaction.ModuleDownloadErrorsKrakenis thrown, we catch it and retry via the outer loop (assuming the user chooses to retry), since the transaction has already been abortedTooManyModsProvideKrakenis thrown, we catch it and retry via the inner loop (assuming the user picks a mod), since the transaction has not been abortedtoUninstall.Clear();is replaced with a newbool didUninstallvariable that allows us to skip already-removed modules without losing track of which ones they were in case we need to re-do that step in the futuretoInstall.Clear();is replaced with a newbool didInstallvariable that allows us to skip already-installed modules without losing track of which ones they were in case we need to re-do that step in the futureAfter these changes: