Installer upgrade behavior more resilient when files are in use and locked #4217
Conversation
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
ramonsmits
changed the title
johnsimons
reviewed
Jun 7, 2024
Member
johnsimons
left a comment
johnsimons
left a comment
There was a problem hiding this comment.
I have added a few comments, but I was thinking that adding a retry with a delay would make things better.
From past experience Antivirus software can cause this kind of locking issues.
Something like https://stackoverflow.com/a/44324346 may be a good start, thoughts?
johnsimons
approved these changes
Jun 10, 2024
Member
johnsimons
left a comment
johnsimons
left a comment
There was a problem hiding this comment.
@ramonsmits LGTM, but I wonder now that you have added retries for the deletion, whether we even need to bother doing the move before hand, something to consider?
Member
Author
|
@johnsimons I've rewritten the installer logic as defined in: |
jpalac
reviewed
Jun 12, 2024
ramonsmits
changed the title
ramonsmits
changed the title
…uired for top directory of the path
…o the user what happened with the folder content
…es that are in use
… trying to move the folder away from /back to the original path
…ly when that complete the folders are swapped and the old version is deleted
…form user what steps failed and if manual steps are required
…ould be on another drive
Co-authored-by: Andreas Bednarz <110360248+abparticular@users.noreply.github.com>
Co-authored-by: Andreas Bednarz <110360248+abparticular@users.noreply.github.com>
Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
…the opposite of DeleteDirectory.
ramonsmits
force-pushed
the
move-before-delete
branch
from
061593a to
7682ca0
Compare
jpalac
approved these changes
Jun 13, 2024
johnsimons
reviewed
Jun 13, 2024
Member
johnsimons
left a comment
johnsimons
left a comment
There was a problem hiding this comment.
I have added a few more comments for consideration
| } | ||
| catch (Exception ex) | ||
| { | ||
| Trace.WriteLine($"ServiceControlInstaller.Engine.Instances.BaseService::PurgeOld Unable to cleanup {oldPath}. Reason: {ex.Message} ({ex.GetType().FullName})"); |
Member
There was a problem hiding this comment.
Why aren't we using the Logging class?
Member
Author
There was a problem hiding this comment.
ramonsmits
commented
Jun 13, 2024
… does not see unhandled exception
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.
It frequently happens that for unknown reasons files are still in use which result in errors like the following:
This change improves the installer logic when upgrading instances by first preparing the new version in a new folder and when that completes swap the folder of the current instance for the folder of the new instance. This resolves most if not all issues around files that are locked or in use.
It also makes the folder delete operation more resilient against very short lock that can be caused because the instance although reporting stopped in Windows Service Control Manager to still be running for a very little time or because a virusscanner locks a file or any other reason for a file to be in use.
Summarized:
IOException10x with a 100ms sleep to allow for short file locksThis is tested with all 3 monitor types by upgrading 5.2.4 to 5.3.0-alpha.0.30 from the build server.