fix a logic error that caused AbandonedMutexException while executing…

… migrations (release-6.4.x) (#4895)

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;

src/NuGet.Core/NuGet.Common/Migrations/MigrationRunner.cs

@@ -2,27 +2,17 @@
 // 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;
 
 namespace NuGet.Common.Migrations
 {
     public static class MigrationRunner
     {
-        private static readonly IReadOnlyList<Action> Migrations = new List<Action>()
-        {
-            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);
 
@@ -32,31 +22,45 @@ 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++)
+                        try
                         {
-                            try
+                            // Only run migrations that have not already been run
+                            if (!File.Exists(expectedMigrationFilename))
                             {
-                                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);
+                                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);
                             }
-                            catch { }
                         }
-
-                        mutex.ReleaseMutex();
+                        catch { }
+                        finally
+                        {
+                            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)
@@ -73,19 +77,5 @@ private 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;
-        }
     }
 }

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}""";
+        }
+    }
+}

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<string> pathsToCheck = new HashSet<string>() { testDirectory.Path, v3cachePath, v3cacheSubDirectoryInfo.FullName };
 
                 Migration1.EnsureExpectedPermissions(pathsToCheck, PosixPermissions.Parse(umask));

test/NuGet.Core.Tests/NuGet.Common.Test/MigrationRunnerTests.cs

@@ -0,0 +1,66 @@
+// 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 NuGet.Common.Migrations;
+using Xunit;
+
+namespace NuGet.Common.Test
+{
+    [CollectionDefinition("MigrationRunner", DisableParallelization = true)]
+    public class MigrationRunnerTests
+    {
+        [Fact]
+        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<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));
+            var files = Directory.GetFiles(directory);
+            Assert.Equal(1, files.Length);
+            Assert.Equal(Path.Combine(directory, "1"), files[0]);
+        }
+    }
+}

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);
+        }
+    }
+}