From cbc10faf082a9626d13212a369f6e1002f46f743 Mon Sep 17 00:00:00 2001 From: Andy Gocke Date: Mon, 7 Dec 2020 19:21:23 -0800 Subject: [PATCH] Change temp folder name used in installer tests (#45523) Tests in the AppHost.Bundle.Tests assembly seem to randomly fail due to a race condition with the file system. They try to create separate '0','1','2'... subdirectories to isolate the published files for each test, but I think what's happening is that files may be marked for deletion, but then not deleted until a later write. For instance, files in '2' may be marked for deletion and some may fail a File.Exists check, which leads to '2' being recreated, at which point deletion may occur, which will cause the current test to fail due to a concurrent write operation. This change tries to avoid locking & contention by randomly generating folder names and using a (hopefully atomically created) lock file to indicate ownership of a particular name. Fixes #43316 --- .../NetCoreApp3CompatModeTests.cs | 23 ++-------- .../AppHost.Bundle.Tests/SingleFileApiTests.cs | 24 ++-------- .../AppHost.Bundle.Tests/SingleFileSharedState.cs | 29 ++++++++++++ src/installer/tests/TestUtils/TestArtifact.cs | 52 +++++++++++++++------- 4 files changed, 73 insertions(+), 55 deletions(-) create mode 100644 src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileSharedState.cs diff --git a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/NetCoreApp3CompatModeTests.cs b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/NetCoreApp3CompatModeTests.cs index dc64be2..95e1061 100644 --- a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/NetCoreApp3CompatModeTests.cs +++ b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/NetCoreApp3CompatModeTests.cs @@ -13,11 +13,11 @@ using Xunit; namespace AppHost.Bundle.Tests { - public class NetCoreApp3CompatModeTests : BundleTestBase, IClassFixture + public class NetCoreApp3CompatModeTests : BundleTestBase, IClassFixture { - private SharedTestState sharedTestState; + private SingleFileSharedState sharedTestState; - public NetCoreApp3CompatModeTests(SharedTestState fixture) + public NetCoreApp3CompatModeTests(SingleFileSharedState fixture) { sharedTestState = fixture; } @@ -25,7 +25,7 @@ namespace AppHost.Bundle.Tests [Fact] public void Bundle_Is_Extracted() { - var fixture = sharedTestState.SingleFileTestFixture.Copy(); + var fixture = sharedTestState.TestFixture.Copy(); UseSingleFileSelfContainedHost(fixture); Bundler bundler = BundleHelper.BundleApp(fixture, out string singleFile, BundleOptions.BundleAllContent); var extractionBaseDir = BundleHelper.GetExtractionRootDir(fixture); @@ -52,20 +52,5 @@ namespace AppHost.Bundle.Tests .ToArray(); extractionDir.Should().HaveFiles(publishedFiles); } - - public class SharedTestState : SharedTestStateBase, IDisposable - { - public TestProjectFixture SingleFileTestFixture { get; set; } - - public SharedTestState() - { - SingleFileTestFixture = PreparePublishedSelfContainedTestProject("SingleFileApiTests"); - } - - public void Dispose() - { - SingleFileTestFixture.Dispose(); - } - } } } diff --git a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileApiTests.cs b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileApiTests.cs index 7121ead..068c8f2 100644 --- a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileApiTests.cs +++ b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileApiTests.cs @@ -8,11 +8,11 @@ using Xunit; namespace AppHost.Bundle.Tests { - public class SingleFileApiTests : BundleTestBase, IClassFixture + public class SingleFileApiTests : BundleTestBase, IClassFixture { - private SharedTestState sharedTestState; + private SingleFileSharedState sharedTestState; - public SingleFileApiTests(SharedTestState fixture) + public SingleFileApiTests(SingleFileSharedState fixture) { sharedTestState = fixture; } @@ -120,23 +120,5 @@ namespace AppHost.Bundle.Tests .And.HaveStdOutContaining(extractionDir) .And.HaveStdOutContaining(bundleDir); } - - public class SharedTestState : SharedTestStateBase, IDisposable - { - public TestProjectFixture TestFixture { get; set; } - - public SharedTestState() - { - // We include mockcoreclr in our project to test native binaries extraction. - string mockCoreClrPath = Path.Combine(RepoDirectories.Artifacts, "corehost_test", - RuntimeInformationExtensions.GetSharedLibraryFileNameForCurrentPlatform("mockcoreclr")); - TestFixture = PreparePublishedSelfContainedTestProject("SingleFileApiTests", $"/p:AddFile={mockCoreClrPath}"); - } - - public void Dispose() - { - TestFixture.Dispose(); - } - } } } diff --git a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileSharedState.cs b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileSharedState.cs new file mode 100644 index 0000000..7051c3f --- /dev/null +++ b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/SingleFileSharedState.cs @@ -0,0 +1,29 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.IO; +using Microsoft.DotNet.CoreSetup.Test; +using Xunit; +using static AppHost.Bundle.Tests.BundleTestBase; + +namespace AppHost.Bundle.Tests +{ + public class SingleFileSharedState : SharedTestStateBase, IDisposable + { + public TestProjectFixture TestFixture { get; set; } + + public SingleFileSharedState() + { + // We include mockcoreclr in our project to test native binaries extraction. + string mockCoreClrPath = Path.Combine(RepoDirectories.Artifacts, "corehost_test", + RuntimeInformationExtensions.GetSharedLibraryFileNameForCurrentPlatform("mockcoreclr")); + TestFixture = PreparePublishedSelfContainedTestProject("SingleFileApiTests", $"/p:AddFile={mockCoreClrPath}"); + } + + public void Dispose() + { + TestFixture.Dispose(); + } + } +} \ No newline at end of file diff --git a/src/installer/tests/TestUtils/TestArtifact.cs b/src/installer/tests/TestUtils/TestArtifact.cs index 517c2f2..8483e4c 100644 --- a/src/installer/tests/TestUtils/TestArtifact.cs +++ b/src/installer/tests/TestUtils/TestArtifact.cs @@ -1,11 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +#nullable enable + using Microsoft.DotNet.CoreSetup.Test.HostActivation; using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; -using System.Threading; namespace Microsoft.DotNet.CoreSetup.Test { @@ -22,7 +24,7 @@ namespace Microsoft.DotNet.CoreSetup.Test { return _repoDirectoriesProvider.Value.GetTestContextVariable(TestArtifactDirectoryEnvironmentVariable) ?? Path.Combine(AppContext.BaseDirectory, TestArtifactDirectoryEnvironmentVariable); - }); + }, isThreadSafe: true); public static bool PreserveTestRuns() => _preserveTestRuns.Value; public static string TestArtifactsPath => _testArtifactsPath.Value; @@ -32,7 +34,7 @@ namespace Microsoft.DotNet.CoreSetup.Test private readonly List _copies = new List(); - public TestArtifact(string location, string name = null) + public TestArtifact(string location, string? name = null) { Location = location; Name = name ?? Path.GetFileName(Location); @@ -57,7 +59,18 @@ namespace Microsoft.DotNet.CoreSetup.Test { if (!PreserveTestRuns() && Directory.Exists(Location)) { - Directory.Delete(Location, true); + try + { + Directory.Delete(Location, true); + + // Delete lock file last + Debug.Assert(!Directory.Exists(Location)); + var lockPath = Directory.GetParent(Location) + ".lock"; + File.Delete(lockPath); + } catch (Exception e) + { + Console.WriteLine("delete failed" + e); + } } foreach (TestArtifact copy in _copies) @@ -68,21 +81,30 @@ namespace Microsoft.DotNet.CoreSetup.Test _copies.Clear(); } - private static readonly object _pathCountLock = new object(); protected static string GetNewTestArtifactPath(string artifactName) { - int projectCount = 0; - string projectCountDir() => Path.Combine(TestArtifactsPath, projectCount.ToString(), artifactName); - - for (; Directory.Exists(projectCountDir()); projectCount++); - - lock (_pathCountLock) + Exception? lastException = null; + for (int i = 0; i < 10; i++) { - string projectDirectory; - for (; Directory.Exists(projectDirectory = projectCountDir()); projectCount++); - FileUtils.EnsureDirectoryExists(projectDirectory); - return projectDirectory; + var parentPath = Path.Combine(TestArtifactsPath, Path.GetRandomFileName()); + // Create a lock file next to the target folder + var lockPath = parentPath + ".lock"; + var artifactPath = Path.Combine(parentPath, artifactName); + try + { + File.Open(lockPath, FileMode.CreateNew, FileAccess.Write).Dispose(); + } + catch (Exception e) + { + // Lock file cannot be created, potential collision + lastException = e; + continue; + } + Directory.CreateDirectory(artifactPath); + return artifactPath; } + Debug.Assert(lastException != null); + throw lastException; } protected static void CopyRecursive(string sourceDirectory, string destinationDirectory, bool overwrite = false) -- 2.7.4