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.

Sign up for GitHub

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

Try to defend against interference by retrying some io operations. #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Try to defend against interference by retrying some io operations. #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 27 additions & 19 deletions src/Squirrel/ReleasePackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,34 +160,42 @@ public string CreateReleasePackage(string outputFile, string packagesRootDir = n
// We must do this, or PathTooLongException may be thrown for some unicode entry names.
public static void ExtractZipDecoded(string zipFilePath, string outFolder, string directoryFilter = null)
{
var zf = new ZipFile(zipFilePath);
Utility.Retry(() =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably work, but it means redoing the whole (100m or so?) extract on every failure. I would have been more inclined to put the Retry calls just (or also) around the directory creation and the individual file extracts.
Will the file extract operations succeed if (on a retry) the file already exists?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought: do we know whether failure to extract/create directory will throw IOException or something else (a permission exception? a Zip utility exception?). May need to pass more exception types to Retry.

{
var zf = new ZipFile(zipFilePath);

foreach (ZipEntry zipEntry in zf) {
if (!zipEntry.IsFile) continue;
foreach(ZipEntry zipEntry in zf)
{
if(!zipEntry.IsFile)
continue;

var entryFileName = Uri.UnescapeDataString(zipEntry.Name);
var entryFileName = Uri.UnescapeDataString(zipEntry.Name);

var buffer = new byte[4096];
var zipStream = zf.GetInputStream(zipEntry);

var fullZipToPath = Path.Combine(outFolder, entryFileName);
var directoryName = Path.GetDirectoryName(fullZipToPath);
var directoryFilter_ = new NameFilter(directoryFilter);
if (directoryFilter_.IsMatch(directoryName)) {
if (directoryName.Length > 0) {
Directory.CreateDirectory(directoryName);
}
var buffer = new byte[4096];
var zipStream = zf.GetInputStream(zipEntry);

using (FileStream streamWriter = File.Create(fullZipToPath)) {
StreamUtils.Copy(zipStream, streamWriter, buffer);
var fullZipToPath = Path.Combine(outFolder, entryFileName);
var directoryName = Path.GetDirectoryName(fullZipToPath);
var directoryFilter_ = new NameFilter(directoryFilter);
if(directoryFilter_.IsMatch(directoryName))
{
if(directoryName.Length > 0)
{
Directory.CreateDirectory(directoryName);
}

using(FileStream streamWriter = File.Create(fullZipToPath))
{
StreamUtils.Copy(zipStream, streamWriter, buffer);
}
}
}
}
zf.Close();
zf.Close();
});
}

// Create zip file with entry names %-encoded, as nupkg file does.
void createZipEncoded(string zipFilePath, string folder)
void createZipEncoded(string zipFilePath, string folder)
{
folder = Path.GetFullPath(folder);
var offset = folder.Length + (folder.EndsWith("\\", StringComparison.OrdinalIgnoreCase) ? 1 : 0);
Expand Down
66 changes: 66 additions & 0 deletions src/Squirrel/RetryUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// From github.com/sillsdev/libpalaso (MIT x11)

using System;
using System.Collections.Generic;
using System.Threading;

namespace Squirrel
{
/// <summary>
/// This utility can be used to wrap any call and retry it in a loop
/// until specific exceptions no longer occur.
///
/// Number of retry attempts, pause time between attempts, and exception types
/// can all be specified or left to the defaults.
///
/// This class came about as an attempt to mitigate certain IO issues, so the
/// defaults are specified along those lines.
/// </summary>
public static class RetryUtility
{
// Nothing special about these numbers. Just attempting to balance issues of user experience.
public const int kDefaultMaxRetryAttempts = 10;
public const int kDefaultRetryDelay = 200;
private static readonly ISet<Type> kDefaultExceptionTypesToRetry = new HashSet<Type> { Type.GetType("System.IO.IOException") };

public static void Retry(Action action, int maxRetryAttempts = kDefaultMaxRetryAttempts, int retryDelay = kDefaultRetryDelay, ISet<Type> exceptionTypesToRetry = null)
{
Retry<object>(() =>
{
action();
return null;
}, maxRetryAttempts, retryDelay, exceptionTypesToRetry);
}

public static T Retry<T>(Func<T> action, int maxRetryAttempts = kDefaultMaxRetryAttempts, int retryDelay = kDefaultRetryDelay, ISet<Type> exceptionTypesToRetry = null)
{
if (exceptionTypesToRetry == null)
exceptionTypesToRetry = kDefaultExceptionTypesToRetry;

for (int attempt = 1; attempt <= maxRetryAttempts; attempt++)
{
try
{
var result = action();
//Debug.WriteLine("Successful after {0} attempts", attempt);
return result;
}
catch (Exception e)
{
if (exceptionTypesToRetry.Contains(e.GetType()))
{
if (attempt == maxRetryAttempts)
{
//Debug.WriteLine("Failed after {0} attempts", attempt);
throw;
}
Thread.Sleep(retryDelay);
continue;
}
throw;
}
}
return default(T);
}
}
}
30 changes: 16 additions & 14 deletions src/Squirrel/UpdateManager.ApplyReleases.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public async Task<string> ApplyReleases(UpdateInfo updateInfo, bool silentInstal
return getDirectoryForRelease(updateInfo.CurrentlyInstalledVersion.Version).FullName;
}

var ret = await this.ErrorIfThrows(() => installPackageToAppDir(updateInfo, release),
var ret = await this.ErrorIfThrows(() => installPackageToAppDir(updateInfo, release),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these trailing-space deletions are distracting, especially in a repo that's mainly someone else's.

"Failed to install package to app dir");
progress(84);

Expand Down Expand Up @@ -125,7 +125,7 @@ public async Task FullUninstall()

if (squirrelAwareApps.Count > 0) {
await squirrelAwareApps.ForEachAsync(async exe => {
using (var cts = new CancellationTokenSource()) {
using (var cts = new CancellationTokenSource()) {
cts.CancelAfter(10 * 1000);

try {
Expand All @@ -139,7 +139,7 @@ await squirrelAwareApps.ForEachAsync(async exe => {
allApps.ForEach(x => RemoveShortcutsForExecutable(x.Name, ShortcutLocation.StartMenu | ShortcutLocation.Desktop));
}

// NB: Some people attempt to uninstall apps while
// NB: Some people attempt to uninstall apps while
// they're still running. I cannot even.
var toKill = allApps
.SelectMany(x => Process.GetProcessesByName(x.Name.Replace(".exe", "")))
Expand All @@ -161,8 +161,8 @@ await squirrelAwareApps.ForEachAsync(async exe => {
await this.ErrorIfThrows(() => Utility.DeleteDirectoryOrJustGiveUp(rootAppDirectory),
"Failed to delete app directory: " + rootAppDirectory);

// NB: We drop this file here so that --checkInstall will ignore
// this folder - if we don't do this, users who "accidentally" run as
// NB: We drop this file here so that --checkInstall will ignore
// this folder - if we don't do this, users who "accidentally" run as
// administrator will find the app reinstalling itself on every
// reboot
if (!Directory.Exists(rootAppDirectory)) {
Expand Down Expand Up @@ -331,15 +331,17 @@ Task<string> installPackageToAppDir(UpdateInfo updateInfo, ReleaseEntry release)

toMove.ForEach(ld => {
ld.GetDirectories()
.ForEachAsync(subdir => subdir.MoveTo(subdir.FullName.Replace(ld.FullName, target.FullName)))
.ForEachAsync(subdir =>
Utility.Retry(() =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, anything we've seen to suggest that other exception types should be retried?

subdir.MoveTo(subdir.FullName.Replace(ld.FullName, target.FullName)), retries:100))
.Wait();

ld.GetFiles()
.ForEachAsync(file => {
var tgt = Path.Combine(target.FullName, file.Name);
this.Log().Info("Moving file {0} to {1}", file.FullName, tgt);
if (File.Exists(tgt)) Utility.DeleteFileHarder(tgt, true);
file.MoveTo(tgt);
Utility.Retry(() => file.MoveTo(tgt), retries: 100);
})
.Wait();
});
Expand Down Expand Up @@ -405,7 +407,7 @@ void executeSelfUpdate(SemanticVersion currentVersion)
return;
}

// If we're running in the context of Update.exe, we can't
// If we're running in the context of Update.exe, we can't
// update ourselves. Instead, ask the new Update.exe to do it
// once we exit
var us = Assembly.GetEntryAssembly();
Expand Down Expand Up @@ -434,7 +436,7 @@ async Task invokePostInstall(SemanticVersion currentVersion, bool isInitialInsta

// For each app, run the install command in-order and wait
if (!firstRunOnly) await squirrelApps.ForEachAsync(async exe => {
using (var cts = new CancellationTokenSource()) {
using (var cts = new CancellationTokenSource()) {
cts.CancelAfter(30 * 1000); // Bloom --allUsers can take quite a while installing Andika

try {
Expand All @@ -445,7 +447,7 @@ async Task invokePostInstall(SemanticVersion currentVersion, bool isInitialInsta
}
}, 1 /* at a time */);

// If this is the first run, we run the apps with first-run and
// If this is the first run, we run the apps with first-run and
// *don't* wait for them, since they're probably the main EXE
if (squirrelApps.Count == 0) {
this.Log().Warn("No apps are marked as Squirrel-aware! Going to run them all");
Expand Down Expand Up @@ -536,7 +538,7 @@ void updateLink(ShellLink shortcut, string newAppPath)
shortcut.WorkingDirectory = newAppPath;
shortcut.Target = target;

// NB: If the executable was in a previous version but not in this
// NB: If the executable was in a previous version but not in this
// one, we should disappear this pin.
if (!File.Exists(target)) {
shortcut.Dispose();
Expand Down Expand Up @@ -621,7 +623,7 @@ async Task cleanDeadVersions(SemanticVersion originalVersion, SemanticVersion cu
this.Log().Info("cleanDeadVersions: exclude folder {0}", currentVersionFolder);
}

// NB: If we try to access a directory that has already been
// NB: If we try to access a directory that has already been
// scheduled for deletion by MoveFileEx it throws what seems like
// NT's only error code, ERROR_ACCESS_DENIED. Squelch errors that
// come from here.
Expand All @@ -638,7 +640,7 @@ await toCleanup.ForEachAsync(async x => {
if (squirrelApps.Count > 0) {
// For each app, run the install command in-order and wait
await squirrelApps.ForEachAsync(async exe => {
using (var cts = new CancellationTokenSource()) {
using (var cts = new CancellationTokenSource()) {
cts.CancelAfter(10 * 1000);

try {
Expand All @@ -663,7 +665,7 @@ await toCleanup.ForEachAsync(async x => {
await Utility.DeleteDirectoryOrJustGiveUp(x.FullName);

if (Directory.Exists(x.FullName)) {
// NB: If we cannot clean up a directory, we need to make
// NB: If we cannot clean up a directory, we need to make
// sure that anyone finding it later won't attempt to run
// Squirrel events on it. We'll mark it with a .dead file
markAppFolderAsDead(x.FullName);
Expand Down
24 changes: 16 additions & 8 deletions src/Squirrel/Utility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ static class Utility
{
public static string RemoveByteOrderMarkerIfPresent(string content)
{
return string.IsNullOrEmpty(content) ?
return string.IsNullOrEmpty(content) ?
string.Empty : RemoveByteOrderMarkerIfPresent(Encoding.UTF8.GetBytes(content));
}

Expand Down Expand Up @@ -331,9 +331,13 @@ public static async Task DeleteDirectory(string directoryPath)
Log().Warn(message, ex);
}

var fileOperations = files.ForEachAsync(file => {
File.SetAttributes(file, FileAttributes.Normal);
File.Delete(file);
var fileOperations = files.ForEachAsync(file =>
{
RetryUtility.Retry(() =>
{
File.SetAttributes(file, FileAttributes.Normal);
File.Delete(file);
});
});

var directoryOperations =
Expand All @@ -344,8 +348,12 @@ public static async Task DeleteDirectory(string directoryPath)
Log().Debug("Now deleting folder: {0}", directoryPath);
File.SetAttributes(directoryPath, FileAttributes.Normal);

try {
Directory.Delete(directoryPath, false);
try
{
RetryUtility.Retry(() =>
{
Directory.Delete(directoryPath, false);
});
} catch (Exception ex) {
var message = String.Format("DeleteDirectory: could not delete - {0}", directoryPath);
Log().ErrorException(message, ex);
Expand All @@ -362,7 +370,7 @@ public static string AppDirForVersion(string rootAppDirectory, SemanticVersion v
return Path.Combine(rootAppDirectory, "app-" + version.ToString());
}

public static string PackageDirectoryForAppDir(string rootAppDirectory)
public static string PackageDirectoryForAppDir(string rootAppDirectory)
{
return Path.Combine(rootAppDirectory, "packages");
}
Expand All @@ -381,7 +389,7 @@ public static IEnumerable<ReleaseEntry> LoadLocalReleases(string localReleaseFil
return ReleaseEntry.ParseReleaseFile(sr.ReadToEnd());
}
}

public static ReleaseEntry FindCurrentVersion(IEnumerable<ReleaseEntry> localReleases)
{
if (!localReleases.Any()) {
Expand Down