From d658c898cc3fc6b429809a96f6f0c879c2213bea Mon Sep 17 00:00:00 2001 From: Kartheek Penagamuri <52756182+kartheekp-ms@users.noreply.github.com> Date: Mon, 24 Oct 2022 11:18:41 -0700 Subject: [PATCH 1/2] fix a logic error that caused AbandonedMutexException while executing migrations (#4859) --- .../NuGet.Common/Migrations/Migration1.cs | 6 +-- .../Migrations/MigrationRunner.cs | 45 ++++++++++++------ .../NuGet.Common/StringExtensions.cs | 13 +++++ .../NuGet.Common.Test/Migration1Tests.cs | 6 +-- .../NuGet.Common.Test/MigrationRunnerTests.cs | 47 +++++++++++++++++++ .../StringExtensionsTests.cs | 30 ++++++++++++ 6 files changed, 127 insertions(+), 20 deletions(-) create mode 100644 src/NuGet.Core/NuGet.Common/StringExtensions.cs create mode 100644 test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs create mode 100644 test/NuGet.Core.Tests/NuGet.Common.Test/StringExtensionsTests.cs diff --git a/src/NuGet.Core/NuGet.Common/Migrations/Migration1.cs b/src/NuGet.Core/NuGet.Common/Migrations/Migration1.cs index f7281c461f5..e0d86cc4c8e 100644 --- a/src/NuGet.Core/NuGet.Common/Migrations/Migration1.cs +++ b/src/NuGet.Core/NuGet.Common/Migrations/Migration1.cs @@ -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; } @@ -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; diff --git a/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs b/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs index 8ce07392c6f..940dc964ad1 100644 --- a/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs +++ b/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs @@ -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) diff --git a/src/NuGet.Core/NuGet.Common/StringExtensions.cs b/src/NuGet.Core/NuGet.Common/StringExtensions.cs new file mode 100644 index 00000000000..6c740ff2b29 --- /dev/null +++ b/src/NuGet.Core/NuGet.Common/StringExtensions.cs @@ -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}"""; + } + } +} diff --git a/test/NuGet.Core.Tests/NuGet.Common.Test/Migration1Tests.cs b/test/NuGet.Core.Tests/NuGet.Common.Test/Migration1Tests.cs index dcd8704ce14..6c2608b88a5 100644 --- a/test/NuGet.Core.Tests/NuGet.Common.Test/Migration1Tests.cs +++ b/test/NuGet.Core.Tests/NuGet.Common.Test/Migration1Tests.cs @@ -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.FormatWithDoubleQuotes()); + Migration1.Exec("chmod", currentPermissions + " " + v3cacheSubDirectoryInfo.FullName.FormatWithDoubleQuotes()); HashSet pathsToCheck = new HashSet() { testDirectory.Path, v3cachePath, v3cacheSubDirectoryInfo.FullName }; Migration1.EnsureExpectedPermissions(pathsToCheck, PosixPermissions.Parse(umask)); diff --git a/test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs b/test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs new file mode 100644 index 00000000000..cff096f7ef5 --- /dev/null +++ b/test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs @@ -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(); + 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); + } + } +} diff --git a/test/NuGet.Core.Tests/NuGet.Common.Test/StringExtensionsTests.cs b/test/NuGet.Core.Tests/NuGet.Common.Test/StringExtensionsTests.cs new file mode 100644 index 00000000000..82b5cf05d5c --- /dev/null +++ b/test/NuGet.Core.Tests/NuGet.Common.Test/StringExtensionsTests.cs @@ -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); + } + } +} From 0acb9ac80e4336e9dd8bb788a4fad68c65ab1c8f Mon Sep 17 00:00:00 2001 From: Kartheek Penagamuri <52756182+kartheekp-ms@users.noreply.github.com> Date: Wed, 26 Oct 2022 11:17:18 -0700 Subject: [PATCH 2/2] refactor NuGet migrations code (#4875) --- .../Migrations/MigrationRunner.cs | 47 ++++--------------- .../NuGet.Common.Test/MigrationRunnerTests.cs | 25 ++++++++-- 2 files changed, 32 insertions(+), 40 deletions(-) diff --git a/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs b/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs index 940dc964ad1..e0250c0905e 100644 --- a/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs +++ b/src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs @@ -2,9 +2,6 @@ // 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.Diagnostics; -using System.Globalization; using System.IO; using System.Threading; @@ -12,17 +9,10 @@ namespace NuGet.Common.Migrations { public static class MigrationRunner { - private static readonly IReadOnlyList Migrations = new List() - { - Migration1.Run - }; private const string MaxMigrationFilename = "1"; public static void Run() { - // since migrations run once per machine, optimize for the scenario where the migration has already run - Debug.Assert(MaxMigrationFilename == Migrations.Count.ToString(CultureInfo.InvariantCulture)); - string migrationsDirectory = GetMigrationsDirectory(); var expectedMigrationFilename = Path.Combine(migrationsDirectory, MaxMigrationFilename); @@ -34,24 +24,21 @@ public static void Run() { if (WaitForMutex(mutex)) { - 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++) + if (!File.Exists(expectedMigrationFilename)) { - 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 { } + Migration1.Run(); + // Create file for the migration run, so that if an older version of NuGet is run, it doesn't try to run migrations again. + File.WriteAllText(expectedMigrationFilename, string.Empty); } } - mutex.ReleaseMutex(); + catch { } + finally + { + mutex.ReleaseMutex(); + } } } } @@ -90,19 +77,5 @@ internal static string GetMigrationsDirectory() Directory.CreateDirectory(migrationsDirectory); return migrationsDirectory; } - - private static int GetHighestMigrationRun(string directory) - { - for (int i = Migrations.Count - 1; i >= 0; --i) - { - var filename = Path.Combine(directory, (i + 1).ToString(CultureInfo.InvariantCulture)); - if (File.Exists(filename)) - { - return i; - } - } - - return -1; - } } } diff --git a/test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs b/test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs index cff096f7ef5..c2560e1fb54 100644 --- a/test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs @@ -5,7 +5,6 @@ using System.Collections.Generic; using System.IO; using System.Threading; -using System.Threading.Tasks; using NuGet.Common.Migrations; using Xunit; @@ -15,7 +14,25 @@ namespace NuGet.Common.Test public class MigrationRunnerTests { [Fact] - public void WhenExecutedInParallelOnlyOneFileIsCreatedForEveryMigration_Success() + public void Run_WhenExecutedOnSingleThreadThenOneMigrationFileIsCreated_Success() + { + // Arrange + string directory = MigrationRunner.GetMigrationsDirectory(); + if (Directory.Exists(directory)) + Directory.Delete(path: directory, recursive: true); + + // Act + MigrationRunner.Run(); + + // Assert + Assert.True(Directory.Exists(directory)); + var files = Directory.GetFiles(directory); + Assert.Equal(1, files.Length); + Assert.Equal(Path.Combine(directory, "1"), files[0]); + } + + [Fact] + public void Run_WhenExecutedInParallelThenOnlyOneMigrationFileIsCreated_Success() { var threads = new List(); int numThreads = 5; @@ -41,7 +58,9 @@ public void WhenExecutedInParallelOnlyOneFileIsCreatedForEveryMigration_Success( // Assert Assert.True(Directory.Exists(directory)); - Assert.Equal(1, Directory.GetFiles(directory).Length); + var files = Directory.GetFiles(directory); + Assert.Equal(1, files.Length); + Assert.Equal(Path.Combine(directory, "1"), files[0]); } } }