From 4cf11b497f1b2e3a4c504e8b525dbb5529764941 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Wed, 12 Jul 2023 13:39:43 -0500 Subject: [PATCH] Fix binlog OOM embedding files Fixes #8595 by storing the embedded-file zip in a temporary directory (instead of memory or binlog target directory) to avoid problems with file watchers. --- .../Logging/BinaryLogger/BinaryLogger.cs | 18 ++++- .../BinaryLogger/BuildEventArgsWriter.cs | 16 ++++ .../BinaryLogger/ProjectImportsCollector.cs | 74 +++++++++---------- src/MSBuild/XMake.cs | 2 +- 4 files changed, 67 insertions(+), 43 deletions(-) diff --git a/src/Build/Logging/BinaryLogger/BinaryLogger.cs b/src/Build/Logging/BinaryLogger/BinaryLogger.cs index 29b259d2bef..28a16df7c9c 100644 --- a/src/Build/Logging/BinaryLogger/BinaryLogger.cs +++ b/src/Build/Logging/BinaryLogger/BinaryLogger.cs @@ -6,6 +6,7 @@ using System.IO.Compression; using Microsoft.Build.Framework; using Microsoft.Build.Shared; +using Microsoft.Build.Shared.FileSystem; #nullable disable @@ -226,12 +227,25 @@ public void Shutdown() if (projectImportsCollector != null) { + projectImportsCollector.Close(); + if (CollectProjectImports == ProjectImportsCollectionMode.Embed) { - eventArgsWriter.WriteBlob(BinaryLogRecordKind.ProjectImportArchive, projectImportsCollector.GetAllBytes()); + var archiveFilePath = projectImportsCollector.ArchiveFilePath; + + // It is possible that the archive couldn't be created for some reason. + // Only embed it if it actually exists. + if (FileSystems.Default.FileExists(archiveFilePath)) + { + using (FileStream fileStream = File.OpenRead(archiveFilePath)) + { + eventArgsWriter.WriteBlob(BinaryLogRecordKind.ProjectImportArchive, fileStream); + } + + File.Delete(archiveFilePath); + } } - projectImportsCollector.Close(); projectImportsCollector = null; } diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs index cf69bcbacbc..7b40c84f4be 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs @@ -218,6 +218,17 @@ public void WriteBlob(BinaryLogRecordKind kind, byte[] bytes) Write(bytes); } + public void WriteBlob(BinaryLogRecordKind kind, Stream stream) + { + // write the blob directly to the underlying writer, + // bypassing the memory stream + using var redirection = RedirectWritesToOriginalWriter(); + + Write(kind); + Write(stream.Length); + Write(stream); + } + /// /// Switches the binaryWriter used by the Write* methods to the direct underlying stream writer /// until the disposable is disposed. Useful to bypass the currentRecordWriter to write a string, @@ -1091,6 +1102,11 @@ private void Write(byte[] bytes) binaryWriter.Write(bytes); } + private void Write(Stream stream) + { + stream.CopyTo(binaryWriter.BaseStream); + } + private void Write(byte b) { binaryWriter.Write(b); diff --git a/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs b/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs index 41fa9daa780..27ededae8cc 100644 --- a/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs +++ b/src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs @@ -7,6 +7,7 @@ using System.IO.Compression; using System.Text; using System.Threading.Tasks; +using Microsoft.Build.Shared; #nullable disable @@ -20,30 +21,10 @@ namespace Microsoft.Build.Logging /// internal class ProjectImportsCollector { - private Stream _stream; - public byte[] GetAllBytes() - { - if (_stream == null) - { - return Array.Empty(); - } - else if (ArchiveFilePath == null) - { - var stream = _stream as MemoryStream; - // Before we can use the zip archive, it must be closed. - Close(false); - return stream.ToArray(); - } - else - { - Close(); - return File.ReadAllBytes(ArchiveFilePath); - } - } - + private Stream _fileStream; private ZipArchive _zipArchive; - private string ArchiveFilePath { get; set; } + public string ArchiveFilePath { get; } /// /// Avoid visiting each file more than once. @@ -55,33 +36,46 @@ public byte[] GetAllBytes() public ProjectImportsCollector(string logFilePath, bool createFile, string sourcesArchiveExtension = ".ProjectImports.zip") { - try + if (createFile) { - if (createFile) - { - ArchiveFilePath = Path.ChangeExtension(logFilePath, sourcesArchiveExtension); - _stream = new FileStream(ArchiveFilePath, FileMode.Create, FileAccess.ReadWrite, FileShare.Delete); - } - else + // Archive file will be stored alongside the binlog + ArchiveFilePath = Path.ChangeExtension(logFilePath, sourcesArchiveExtension); + } + else + { + string cacheDirectory = FileUtilities.GetCacheDirectory(); + if (!Directory.Exists(cacheDirectory)) { - _stream = new MemoryStream(); + Directory.CreateDirectory(cacheDirectory); } - _zipArchive = new ZipArchive(_stream, ZipArchiveMode.Create, true); + + // Archive file will be temporarily stored in MSBuild cache folder and deleted when no longer needed + ArchiveFilePath = Path.Combine( + cacheDirectory, + Path.ChangeExtension( + Path.GetFileName(logFilePath), + sourcesArchiveExtension)); + } + + try + { + _fileStream = new FileStream(ArchiveFilePath, FileMode.Create, FileAccess.ReadWrite, FileShare.Delete); + _zipArchive = new ZipArchive(_fileStream, ZipArchiveMode.Create); } catch { // For some reason we weren't able to create a file for the archive. // Disable the file collector. - _stream = null; + _fileStream = null; _zipArchive = null; } } public void AddFile(string filePath) { - if (filePath != null && _stream != null) + if (filePath != null && _fileStream != null) { - lock (_stream) + lock (_fileStream) { // enqueue the task to add a file and return quickly // to avoid holding up the current thread @@ -101,9 +95,9 @@ public void AddFile(string filePath) public void AddFileFromMemory(string filePath, string data) { - if (filePath != null && data != null && _stream != null) + if (filePath != null && data != null && _fileStream != null) { - lock (_stream) + lock (_fileStream) { // enqueue the task to add a file and return quickly // to avoid holding up the current thread @@ -197,7 +191,7 @@ private static string CalculateArchivePath(string filePath) return archivePath; } - public void Close(bool closeStream = true) + public void Close() { // wait for all pending file writes to complete _currentTask.Wait(); @@ -208,10 +202,10 @@ public void Close(bool closeStream = true) _zipArchive = null; } - if (closeStream && (_stream != null)) + if (_fileStream != null) { - _stream.Dispose(); - _stream = null; + _fileStream.Dispose(); + _fileStream = null; } } } diff --git a/src/MSBuild/XMake.cs b/src/MSBuild/XMake.cs index 06c53027f78..fb86829c8d1 100644 --- a/src/MSBuild/XMake.cs +++ b/src/MSBuild/XMake.cs @@ -1498,8 +1498,8 @@ internal static bool BuildProject( } finally { - FileUtilities.ClearCacheDirectory(); projectCollection?.Dispose(); + FileUtilities.ClearCacheDirectory(); // Build manager shall be reused for all build sessions. // If, for one reason or another, this behavior needs to change in future