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

fix a logic error that caused AbandonedMutexException while executing migrations #4859

Merged
merged 11 commits into from
Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
6 changes: 3 additions & 3 deletions src/NuGet.Core/NuGet.Common/Migrations/Migration1.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ void AddAllParentDirectoriesUpToHome(string path)
{
pathsToCheck.Add(path);

if (!path.StartsWith(homePath, StringComparison.Ordinal))
if (!path.StartsWith(homePath + Path.DirectorySeparatorChar, StringComparison.Ordinal))
{
return;
}
Expand Down Expand Up @@ -156,13 +156,13 @@ private static void FixPermissions(string path, PosixPermissions umask)
{
PosixPermissions correctedPermissions = permissions.Value.WithUmask(umask);
string correctedPermissionsString = correctedPermissions.ToString();
Exec("chmod", correctedPermissionsString + " " + path);
Exec("chmod", correctedPermissionsString + " " + path.FormatWithDoubleQuotes());
}
}

internal static PosixPermissions? GetPermissions(string path)
{
string output = Exec("ls", "-ld " + path);
string output = Exec("ls", "-ld " + path.FormatWithDoubleQuotes());
if (output == null)
{
return null;
Expand Down
45 changes: 31 additions & 14 deletions src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,31 +32,48 @@ public static void Run()
// so use a global mutex and then check if someone else already did the work.
using (var mutex = new Mutex(false, "NuGet-Migrations"))
{
bool signal = mutex.WaitOne(TimeSpan.FromMinutes(1), false);
if (signal && !File.Exists(expectedMigrationFilename))
if (WaitForMutex(mutex))
{
// Only run migrations that have not already been run
int highestMigrationRun = GetHighestMigrationRun(migrationsDirectory);
for (int i = highestMigrationRun + 1; i < Migrations.Count; i++)
if (!File.Exists(expectedMigrationFilename))
{
try
// Only run migrations that have not already been run
int highestMigrationRun = GetHighestMigrationRun(migrationsDirectory);
for (int i = highestMigrationRun + 1; i < Migrations.Count; i++)
{
Migrations[i]();
// Create file for every migration run, so that if an older version of NuGet is run, it doesn't try to run
// migrations again.
string migrationFile = Path.Combine(migrationsDirectory, (i + 1).ToString(CultureInfo.InvariantCulture));
File.WriteAllText(migrationFile, string.Empty);
try
{
Migrations[i]();
// Create file for every migration run, so that if an older version of NuGet is run, it doesn't try to run
// migrations again.
string migrationFile = Path.Combine(migrationsDirectory, (i + 1).ToString(CultureInfo.InvariantCulture));
File.WriteAllText(migrationFile, string.Empty);
}
catch { }
}
catch { }
}

mutex.ReleaseMutex();
}
}
}

static bool WaitForMutex(Mutex mutex)
{
bool captured;

try
{
captured = mutex.WaitOne(TimeSpan.FromMinutes(1), false);
}
catch (AbandonedMutexException)
{
captured = true;
}

return captured;
}
}

private static string GetMigrationsDirectory()
internal static string GetMigrationsDirectory()
{
string migrationsDirectory;
if (RuntimeEnvironmentHelper.IsWindows)
Expand Down
13 changes: 13 additions & 0 deletions src/NuGet.Core/NuGet.Common/StringExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace NuGet.Common
{
internal static class StringExtensions
{
internal static string FormatWithDoubleQuotes(this string s)
{
return s == null ? s : $@"""{s}""";
}
}
}
6 changes: 3 additions & 3 deletions test/NuGet.Core.Tests/NuGet.Common.Test/Migration1Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ public void EnsureExpectedPermissions_Directories_Success(string currentPermissi
{
using (var testDirectory = TestDirectory.Create())
{
var v3cachePath = Path.Combine(testDirectory, "v3-cache");
var v3cachePath = Path.Combine(testDirectory, "my documents", "v3-cache");
var v3cacheSubDirectoryInfo = Directory.CreateDirectory(Path.Combine(v3cachePath, "subDirectory"));
Migration1.Exec("chmod", currentPermissions + " " + testDirectory.Path);
Migration1.Exec("chmod", currentPermissions + " " + v3cachePath);
Migration1.Exec("chmod", currentPermissions + " " + v3cacheSubDirectoryInfo.FullName);
Migration1.Exec("chmod", currentPermissions + " " + "\"" + v3cachePath + "\"");
Migration1.Exec("chmod", currentPermissions + " " + "\"" + v3cacheSubDirectoryInfo.FullName + "\"");
HashSet<string> pathsToCheck = new HashSet<string>() { testDirectory.Path, v3cachePath, v3cacheSubDirectoryInfo.FullName };

Migration1.EnsureExpectedPermissions(pathsToCheck, PosixPermissions.Parse(umask));
Expand Down
47 changes: 47 additions & 0 deletions test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Common.Migrations;
using Xunit;

namespace NuGet.Common.Test
{
[CollectionDefinition("MigrationRunner", DisableParallelization = true)]
public class MigrationRunnerTests
{
[Fact]
public void WhenExecutedInParallelOnlyOneFileIsCreatedForEveryMigration_Success()
{
var threads = new List<Thread>();
int numThreads = 5;
int timeoutInSeconds = 90;

// Arrange
string directory = MigrationRunner.GetMigrationsDirectory();
if (Directory.Exists(directory))
Directory.Delete(path: directory, recursive: true);

// Act
for (int i = 0; i < numThreads; i++)
{
var thread = new Thread(MigrationRunner.Run);
thread.Start();
threads.Add(thread);
}

foreach (var thread in threads)
{
thread.Join(timeout: TimeSpan.FromSeconds(timeoutInSeconds));
}

// Assert
Assert.True(Directory.Exists(directory));
Assert.Equal(1, Directory.GetFiles(directory).Length);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Xunit;

namespace NuGet.Common.Test
{
public class StringExtensionsTests
{
[Fact]
public void FormatWithDoubleQuotes_WhenNullIsPassedReturnsNull_Success()
{
string actual = null;
string formatted = actual.FormatWithDoubleQuotes();
Assert.Equal(actual, formatted);
}

[Theory]
[InlineData("")]
[InlineData("/home/user/NuGet AppData/share")]
[InlineData("/home/user/NuGet/share")]
[InlineData(@"c:\users\NuGet App\Share")]
[InlineData(@"c:\users\NuGet\Share")]
public void FormatWithDoubleQuotes_WhenNonNullStringIsPassedReturnsFormattedString_Success(string actual)
{
string formatted = actual.FormatWithDoubleQuotes();
Assert.Equal($@"""{actual}""", formatted);
}
}
}