From b95e523a3003a5744506239ceaac2aafa3ac9a9d Mon Sep 17 00:00:00 2001 From: Swaroop Sridhar Date: Fri, 14 Feb 2020 15:38:20 -0800 Subject: [PATCH] SingleFile bundles: Ensure extraction mappings are closed on Windows. (#2272) When running a single-file app, the AppHost mmap()s itself in order to read its contents and extract the embedded contents. The Apphost must always map its contents in order to read the headers, but doesn't always extract the contents, because previously extracted files are re-used when available. In the case where apphost doesn't extract, the file mapping wasn't immediately closed on Windows. This prevents the app from being renamed while running -- an idiom used while updating the app in-place. --- .../corehost/cli/apphost/bundle/runner.cpp | 10 +- .../corehost/cli/hostmisc/pal.unix.cpp | 6 +- .../corehost/cli/hostmisc/pal.windows.cpp | 18 ++-- .../AppWithWait/AppWithWait.csproj | 10 ++ .../TestProjects/AppWithWait/Program.cs | 35 +++++++ .../AppHost.Bundle.Tests/BundleRename.cs | 91 +++++++++++++++++++ .../Helpers/BundleHelper.cs | 5 + .../test/TestUtils/TestProjectFixture.cs | 9 +- 8 files changed, 166 insertions(+), 18 deletions(-) create mode 100644 src/installer/test/Assets/TestProjects/AppWithWait/AppWithWait.csproj create mode 100644 src/installer/test/Assets/TestProjects/AppWithWait/Program.cs create mode 100644 src/installer/test/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleRename.cs diff --git a/src/installer/corehost/cli/apphost/bundle/runner.cpp b/src/installer/corehost/cli/apphost/bundle/runner.cpp index ccae5ed2949..14862046a95 100644 --- a/src/installer/corehost/cli/apphost/bundle/runner.cpp +++ b/src/installer/corehost/cli/apphost/bundle/runner.cpp @@ -50,14 +50,12 @@ StatusCode runner_t::extract() m_extraction_dir = extractor.extraction_dir(); // Determine if embedded files are already extracted, and available for reuse - if (extractor.can_reuse_extraction()) + if (!extractor.can_reuse_extraction()) { - return StatusCode::Success; - } - - manifest_t manifest = manifest_t::read(reader, header.num_embedded_files()); + manifest_t manifest = manifest_t::read(reader, header.num_embedded_files()); - extractor.extract(manifest, reader); + extractor.extract(manifest, reader); + } unmap_host(); diff --git a/src/installer/corehost/cli/hostmisc/pal.unix.cpp b/src/installer/corehost/cli/hostmisc/pal.unix.cpp index 2c60b5b46fe..54488e6bfdd 100644 --- a/src/installer/corehost/cli/hostmisc/pal.unix.cpp +++ b/src/installer/corehost/cli/hostmisc/pal.unix.cpp @@ -63,14 +63,14 @@ void* pal::map_file_readonly(const pal::string_t& path, size_t& length) int fd = open(path.c_str(), O_RDONLY, (S_IRUSR | S_IRGRP | S_IROTH)); if (fd == -1) { - trace::warning(_X("Failed to map file. open(%s) failed with error %d"), path.c_str(), errno); + trace::error(_X("Failed to map file. open(%s) failed with error %d"), path.c_str(), errno); return nullptr; } struct stat buf; if (fstat(fd, &buf) != 0) { - trace::warning(_X("Failed to map file. fstat(%s) failed with error %d"), path.c_str(), errno); + trace::error(_X("Failed to map file. fstat(%s) failed with error %d"), path.c_str(), errno); close(fd); return nullptr; } @@ -80,7 +80,7 @@ void* pal::map_file_readonly(const pal::string_t& path, size_t& length) if(address == nullptr) { - trace::warning(_X("Failed to map file. mmap(%s) failed with error %d"), path.c_str(), errno); + trace::error(_X("Failed to map file. mmap(%s) failed with error %d"), path.c_str(), errno); close(fd); return nullptr; } diff --git a/src/installer/corehost/cli/hostmisc/pal.windows.cpp b/src/installer/corehost/cli/hostmisc/pal.windows.cpp index 54b9a529ac2..1e0ff1d985b 100644 --- a/src/installer/corehost/cli/hostmisc/pal.windows.cpp +++ b/src/installer/corehost/cli/hostmisc/pal.windows.cpp @@ -82,14 +82,14 @@ void* pal::map_file_readonly(const pal::string_t& path, size_t &length) if (file == INVALID_HANDLE_VALUE) { - trace::warning(_X("Failed to map file. CreateFileW(%s) failed with error %d"), path.c_str(), GetLastError()); + trace::error(_X("Failed to map file. CreateFileW(%s) failed with error %d"), path.c_str(), GetLastError()); return nullptr; } LARGE_INTEGER fileSize; if (GetFileSizeEx(file, &fileSize) == 0) { - trace::warning(_X("Failed to map file. GetFileSizeEx(%s) failed with error %d"), path.c_str(), GetLastError()); + trace::error(_X("Failed to map file. GetFileSizeEx(%s) failed with error %d"), path.c_str(), GetLastError()); CloseHandle(file); return nullptr; } @@ -99,20 +99,24 @@ void* pal::map_file_readonly(const pal::string_t& path, size_t &length) if (map == NULL) { - trace::warning(_X("Failed to map file. CreateFileMappingW(%s) failed with error %d"), path.c_str(), GetLastError()); + trace::error(_X("Failed to map file. CreateFileMappingW(%s) failed with error %d"), path.c_str(), GetLastError()); CloseHandle(file); return nullptr; } void *address = MapViewOfFile(map, FILE_MAP_READ, 0, 0, 0); - if (map == NULL) + if (address == NULL) { - trace::warning(_X("Failed to map file. MapViewOfFile(%s) failed with error %d"), path.c_str(), GetLastError()); - CloseHandle(file); - return nullptr; + trace::error(_X("Failed to map file. MapViewOfFile(%s) failed with error %d"), path.c_str(), GetLastError()); } + // The file-handle (file) and mapping object handle (map) can be safely closed + // once the file is mapped. The OS keeps the file open if there is an open mapping into the file. + + CloseHandle(map); + CloseHandle(file); + return address; } diff --git a/src/installer/test/Assets/TestProjects/AppWithWait/AppWithWait.csproj b/src/installer/test/Assets/TestProjects/AppWithWait/AppWithWait.csproj new file mode 100644 index 00000000000..59e3b57746c --- /dev/null +++ b/src/installer/test/Assets/TestProjects/AppWithWait/AppWithWait.csproj @@ -0,0 +1,10 @@ + + + + $(NETCoreAppFramework) + Exe + $(TestTargetRid) + $(MNAVersion) + + + diff --git a/src/installer/test/Assets/TestProjects/AppWithWait/Program.cs b/src/installer/test/Assets/TestProjects/AppWithWait/Program.cs new file mode 100644 index 00000000000..7a39929f707 --- /dev/null +++ b/src/installer/test/Assets/TestProjects/AppWithWait/Program.cs @@ -0,0 +1,35 @@ +using System; +using System.IO; +using System.Threading; + +namespace AppWithSubDirs +{ + public static class Program + { + public static void Main(string[] args) + { + Console.Write("Hello "); + + // If the caller wants the app to start and wait, + // it provides the name of a lock-file to write. + // In this case, this test app creates the lock file + // and waits until the file is deleted. + if (args.Length > 0) + { + string writeFile = args[0]; + + var fs = File.Create(writeFile); + fs.Close(); + + Thread.Sleep(200); + + while (File.Exists(writeFile)) + { + Thread.Sleep(100); + } + } + + Console.WriteLine("World!"); + } + } +} diff --git a/src/installer/test/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleRename.cs b/src/installer/test/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleRename.cs new file mode 100644 index 00000000000..07efcceff6e --- /dev/null +++ b/src/installer/test/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleRename.cs @@ -0,0 +1,91 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.IO; +using Xunit; +using Microsoft.DotNet.Cli.Build.Framework; +using Microsoft.DotNet.CoreSetup.Test; +using BundleTests.Helpers; +using System.Threading; + +namespace AppHost.Bundle.Tests +{ + public class BundleRename : IClassFixture + { + private SharedTestState sharedTestState; + + public BundleRename(SharedTestState fixture) + { + sharedTestState = fixture; + } + + [Theory] + [InlineData(true)] // Test renaming the single-exe during the initial run, when contents are extracted + [InlineData(false)] // Test renaming the single-exe during subsequent runs, when contents are reused + private void Bundle_can_be_renamed_while_running(bool renameFirstRun) + { + var fixture = sharedTestState.TestFixture.Copy(); + string singleFile = BundleHelper.GetPublishedSingleFilePath(fixture); + string renameFile = Path.Combine(BundleHelper.GetPublishPath(fixture), Path.GetRandomFileName()); + string writeFile = Path.Combine(BundleHelper.GetPublishPath(fixture), "lock"); + + if (!renameFirstRun) + { + Command.Create(singleFile) + .CaptureStdErr() + .CaptureStdOut() + .Execute() + .Should() + .Pass() + .And + .HaveStdOutContaining("Hello World!"); + } + + var singleExe = Command.Create(singleFile, writeFile) + .CaptureStdErr() + .CaptureStdOut() + .Start(); + + // Once the App starts running, it creates the writeFile, and waits until the file is deleted. + while (!File.Exists(writeFile)) + { + Thread.Sleep(100); + } + + File.Move(singleFile, renameFile); + File.Delete(writeFile); + + var result = singleExe.WaitForExit(fExpectedToFail: false); + + result + .Should() + .Pass() + .And + .HaveStdOutContaining("Hello World!"); + } + + public class SharedTestState : IDisposable + { + public TestProjectFixture TestFixture { get; set; } + public RepoDirectoriesProvider RepoDirectories { get; set; } + + public SharedTestState() + { + RepoDirectories = new RepoDirectoriesProvider(); + TestFixture = new TestProjectFixture("AppWithWait", RepoDirectories); + TestFixture + .EnsureRestoredForRid(TestFixture.CurrentRid, RepoDirectories.CorehostPackages) + .PublishProject(runtime: TestFixture.CurrentRid, + singleFile: true, + outputDirectory: BundleHelper.GetPublishPath(TestFixture)); + } + + public void Dispose() + { + TestFixture.Dispose(); + } + } + } +} diff --git a/src/installer/test/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs b/src/installer/test/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs index 5d245740edb..81b8a7ef2ed 100644 --- a/src/installer/test/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs +++ b/src/installer/test/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs @@ -22,6 +22,11 @@ namespace BundleTests.Helpers return Path.Combine(GetPublishPath(fixture), GetAppName(fixture)); } + public static string GetPublishedSingleFilePath(TestProjectFixture fixture) + { + return GetHostPath(fixture); + } + public static string GetHostName(TestProjectFixture fixture) { return Path.GetFileName(fixture.TestProject.AppExe); diff --git a/src/installer/test/TestUtils/TestProjectFixture.cs b/src/installer/test/TestUtils/TestProjectFixture.cs index a762d2930d4..13f5b8b0199 100644 --- a/src/installer/test/TestUtils/TestProjectFixture.cs +++ b/src/installer/test/TestUtils/TestProjectFixture.cs @@ -6,7 +6,6 @@ using Microsoft.DotNet.Cli.Build; using System; using System.Collections.Generic; using System.IO; -using System.Text; namespace Microsoft.DotNet.CoreSetup.Test { @@ -252,7 +251,8 @@ namespace Microsoft.DotNet.CoreSetup.Test string runtime = null, string framework = null, string selfContained = null, - string outputDirectory = null) + string outputDirectory = null, + bool singleFile = false) { dotnet = dotnet ?? SdkDotnet; outputDirectory = outputDirectory ?? TestProject.OutputDirectory; @@ -291,6 +291,11 @@ namespace Microsoft.DotNet.CoreSetup.Test publishArgs.Add(outputDirectory); } + if (singleFile) + { + publishArgs.Add("/p:PublishSingleFile=true"); + } + publishArgs.Add($"/p:TestTargetRid={RepoDirProvider.TargetRID}"); publishArgs.Add($"/p:MNAVersion={RepoDirProvider.MicrosoftNETCoreAppVersion}"); -- 2.34.1