From 95835a566116351813b5eb669fc3c3f1444f4f35 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Tue, 9 Apr 2024 19:43:40 +0200 Subject: [PATCH 01/20] add dispose part 1 --- .../BackEnd/BuildManager/BuildManager.cs | 116 ++++++++-------- src/Build/BackEnd/Node/OutOfProcServerNode.cs | 2 + .../Shared/BuildRequestConfiguration.cs | 28 ++-- src/Build/BackEnd/Shared/TargetResult.cs | 22 ++- src/Framework/NativeMethods.cs | 131 +++++++++--------- src/MSBuild/XMake.cs | 41 ++++-- 6 files changed, 187 insertions(+), 153 deletions(-) diff --git a/src/Build/BackEnd/BuildManager/BuildManager.cs b/src/Build/BackEnd/BuildManager/BuildManager.cs index 33fba22ca1e..98c7f61606c 100644 --- a/src/Build/BackEnd/BuildManager/BuildManager.cs +++ b/src/Build/BackEnd/BuildManager/BuildManager.cs @@ -2004,82 +2004,84 @@ private Dictionary BuildGraph( IReadOnlyDictionary> targetsPerNode, GraphBuildRequestData graphBuildRequestData) { - var waitHandle = new AutoResetEvent(true); - var graphBuildStateLock = new object(); - - var blockedNodes = new HashSet(projectGraph.ProjectNodes); - var finishedNodes = new HashSet(projectGraph.ProjectNodes.Count); - var buildingNodes = new Dictionary(); var resultsPerNode = new Dictionary(projectGraph.ProjectNodes.Count); - ExceptionDispatchInfo submissionException = null; - - while (blockedNodes.Count > 0 || buildingNodes.Count > 0) + using (var waitHandle = new AutoResetEvent(true)) { - waitHandle.WaitOne(); + var graphBuildStateLock = new object(); - // When a cache plugin is present, ExecuteSubmission(BuildSubmission) executes on a separate thread whose exceptions do not get observed. - // Observe them here to keep the same exception flow with the case when there's no plugins and ExecuteSubmission(BuildSubmission) does not run on a separate thread. - if (submissionException != null) - { - submissionException.Throw(); - } + var blockedNodes = new HashSet(projectGraph.ProjectNodes); + var finishedNodes = new HashSet(projectGraph.ProjectNodes.Count); + var buildingNodes = new Dictionary(); + ExceptionDispatchInfo submissionException = null; - lock (graphBuildStateLock) + while (blockedNodes.Count > 0 || buildingNodes.Count > 0) { - var unblockedNodes = blockedNodes - .Where(node => node.ProjectReferences.All(projectReference => finishedNodes.Contains(projectReference))) - .ToList(); - foreach (var node in unblockedNodes) + waitHandle.WaitOne(); + + // When a cache plugin is present, ExecuteSubmission(BuildSubmission) executes on a separate thread whose exceptions do not get observed. + // Observe them here to keep the same exception flow with the case when there's no plugins and ExecuteSubmission(BuildSubmission) does not run on a separate thread. + if (submissionException != null) { - var targetList = targetsPerNode[node]; - if (targetList.Count == 0) + submissionException.Throw(); + } + + lock (graphBuildStateLock) + { + var unblockedNodes = blockedNodes + .Where(node => node.ProjectReferences.All(projectReference => finishedNodes.Contains(projectReference))) + .ToList(); + foreach (var node in unblockedNodes) { - // An empty target list here means "no targets" instead of "default targets", so don't even build it. - finishedNodes.Add(node); - blockedNodes.Remove(node); + var targetList = targetsPerNode[node]; + if (targetList.Count == 0) + { + // An empty target list here means "no targets" instead of "default targets", so don't even build it. + finishedNodes.Add(node); + blockedNodes.Remove(node); - waitHandle.Set(); + waitHandle.Set(); - continue; - } + continue; + } - var request = new BuildRequestData( - node.ProjectInstance, - targetList.ToArray(), - graphBuildRequestData.HostServices, - graphBuildRequestData.Flags); - - // TODO Tack onto the existing submission instead of pending a whole new submission for every node - // Among other things, this makes BuildParameters.DetailedSummary produce a summary for each node, which is not desirable. - // We basically want to submit all requests to the scheduler all at once and describe dependencies by requests being blocked by other requests. - // However today the scheduler only keeps track of MSBuild nodes being blocked by other MSBuild nodes, and MSBuild nodes haven't been assigned to the graph nodes yet. - var innerBuildSubmission = PendBuildRequest(request); - buildingNodes.Add(innerBuildSubmission, node); - blockedNodes.Remove(node); - innerBuildSubmission.ExecuteAsync(finishedBuildSubmission => - { - lock (graphBuildStateLock) + var request = new BuildRequestData( + node.ProjectInstance, + targetList.ToArray(), + graphBuildRequestData.HostServices, + graphBuildRequestData.Flags); + + // TODO Tack onto the existing submission instead of pending a whole new submission for every node + // Among other things, this makes BuildParameters.DetailedSummary produce a summary for each node, which is not desirable. + // We basically want to submit all requests to the scheduler all at once and describe dependencies by requests being blocked by other requests. + // However today the scheduler only keeps track of MSBuild nodes being blocked by other MSBuild nodes, and MSBuild nodes haven't been assigned to the graph nodes yet. + var innerBuildSubmission = PendBuildRequest(request); + buildingNodes.Add(innerBuildSubmission, node); + blockedNodes.Remove(node); + innerBuildSubmission.ExecuteAsync(finishedBuildSubmission => { - if (submissionException == null && finishedBuildSubmission.BuildResult.Exception != null) + lock (graphBuildStateLock) { - // Preserve the original stack. - submissionException = ExceptionDispatchInfo.Capture(finishedBuildSubmission.BuildResult.Exception); - } + if (submissionException == null && finishedBuildSubmission.BuildResult.Exception != null) + { + // Preserve the original stack. + submissionException = ExceptionDispatchInfo.Capture(finishedBuildSubmission.BuildResult.Exception); + } - ProjectGraphNode finishedNode = buildingNodes[finishedBuildSubmission]; + ProjectGraphNode finishedNode = buildingNodes[finishedBuildSubmission]; - finishedNodes.Add(finishedNode); - buildingNodes.Remove(finishedBuildSubmission); + finishedNodes.Add(finishedNode); + buildingNodes.Remove(finishedBuildSubmission); - resultsPerNode.Add(finishedNode, finishedBuildSubmission.BuildResult); - } + resultsPerNode.Add(finishedNode, finishedBuildSubmission.BuildResult); + } - waitHandle.Set(); - }, null); + waitHandle.Set(); + }, null); + } } } } - + return resultsPerNode; } diff --git a/src/Build/BackEnd/Node/OutOfProcServerNode.cs b/src/Build/BackEnd/Node/OutOfProcServerNode.cs index 6b2715903e7..54a45f77816 100644 --- a/src/Build/BackEnd/Node/OutOfProcServerNode.cs +++ b/src/Build/BackEnd/Node/OutOfProcServerNode.cs @@ -430,6 +430,7 @@ private void HandleServerNodeBuildCommand(ServerNodeBuildCommand command) _shutdownReason = _cancelRequested ? NodeEngineShutdownReason.BuildComplete : NodeEngineShutdownReason.BuildCompleteReuse; _shutdownEvent.Set(); } + internal sealed class RedirectConsoleWriter : StringWriter { private readonly Action _writeCallback; @@ -446,6 +447,7 @@ private RedirectConsoleWriter(Action writeCallback) public static TextWriter Create(Action writeCallback) { RedirectConsoleWriter writer = new(writeCallback); + return writer._syncWriter; } diff --git a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs index eda42874f86..f34b0443210 100644 --- a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs +++ b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs @@ -698,7 +698,11 @@ public void CacheIfPossible() { if (IsCacheable) { - using ITranslator translator = GetConfigurationTranslator(TranslationDirection.WriteToStream); + string cacheFile = GetCacheFile(); + Directory.CreateDirectory(Path.GetDirectoryName(cacheFile)); + + using Stream stream = File.Create(cacheFile); + using ITranslator translator = GetConfigurationTranslator(TranslationDirection.WriteToStream, stream, cacheFile); _project.Cache(translator); _baseLookup = null; @@ -726,7 +730,9 @@ public void RetrieveFromCache() return; } - using ITranslator translator = GetConfigurationTranslator(TranslationDirection.ReadFromStream); + string cacheFile = GetCacheFile(); + using Stream stream = File.OpenRead(cacheFile); + using ITranslator translator = GetConfigurationTranslator(TranslationDirection.ReadFromStream, stream, cacheFile); _project.RetrieveFromCache(translator); @@ -1024,23 +1030,17 @@ private static string ResolveToolsVersion(BuildRequestData data, string defaultT /// /// Gets the translator for this configuration. /// - private ITranslator GetConfigurationTranslator(TranslationDirection direction) + private ITranslator GetConfigurationTranslator(TranslationDirection direction, Stream stream, string cacheFile) { - string cacheFile = GetCacheFile(); try { - if (direction == TranslationDirection.WriteToStream) - { - Directory.CreateDirectory(Path.GetDirectoryName(cacheFile)); - return BinaryTranslator.GetWriteTranslator(File.Create(cacheFile)); - } - else - { + return direction == TranslationDirection.WriteToStream + ? BinaryTranslator.GetWriteTranslator(stream) + // Not using sharedReadBuffer because this is not a memory stream and so the buffer won't be used anyway. - return BinaryTranslator.GetReadTranslator(File.OpenRead(cacheFile), InterningBinaryReader.PoolingBuffer); - } + : BinaryTranslator.GetReadTranslator(stream, InterningBinaryReader.PoolingBuffer); } - catch (Exception e) when (e is DirectoryNotFoundException || e is UnauthorizedAccessException) + catch (Exception e) when (e is DirectoryNotFoundException or UnauthorizedAccessException) { ErrorUtilities.ThrowInvalidOperation("CacheFileInaccessible", cacheFile, e); throw; diff --git a/src/Build/BackEnd/Shared/TargetResult.cs b/src/Build/BackEnd/Shared/TargetResult.cs index d435d1c3606..b490f005543 100644 --- a/src/Build/BackEnd/Shared/TargetResult.cs +++ b/src/Build/BackEnd/Shared/TargetResult.cs @@ -10,6 +10,7 @@ using Microsoft.Build.Framework; using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; +using static BuildXL.Processes.SandboxConnectionKext; using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem; #nullable disable @@ -245,7 +246,11 @@ internal void CacheItems(int configId, string targetName) return; } - using ITranslator translator = GetResultsCacheTranslator(configId, targetName, TranslationDirection.WriteToStream); + string cacheFile = GetCacheFile(configId, targetName); + Directory.CreateDirectory(Path.GetDirectoryName(cacheFile)); + + using Stream stream = File.Create(cacheFile); + using ITranslator translator = GetResultsCacheTranslator(TranslationDirection.WriteToStream, stream, cacheFile); // If the translator is null, it means these results were cached once before. Since target results are immutable once they // have been created, there is no point in writing them again. @@ -279,7 +284,9 @@ private void RetrieveItemsFromCache() { if (_items == null) { - using ITranslator translator = GetResultsCacheTranslator(_cacheInfo.ConfigId, _cacheInfo.TargetName, TranslationDirection.ReadFromStream); + string cacheFile = GetCacheFile(_cacheInfo.ConfigId, _cacheInfo.TargetName); + using Stream stream = File.OpenRead(cacheFile); + using ITranslator translator = GetResultsCacheTranslator(TranslationDirection.ReadFromStream, stream, cacheFile); TranslateItems(translator); _cacheInfo = new CacheInfo(); @@ -339,23 +346,24 @@ private void TranslateItems(ITranslator translator) /// /// Gets the translator for this configuration. /// - private static ITranslator GetResultsCacheTranslator(int configId, string targetToCache, TranslationDirection direction) + private static ITranslator GetResultsCacheTranslator( + TranslationDirection direction, + Stream stream, + string cacheFile) { - string cacheFile = GetCacheFile(configId, targetToCache); if (direction == TranslationDirection.WriteToStream) { - Directory.CreateDirectory(Path.GetDirectoryName(cacheFile)); if (FileSystems.Default.FileExists(cacheFile)) { // If the file already exists, then we have cached this once before. No need to cache it again since it cannot have changed. return null; } - return BinaryTranslator.GetWriteTranslator(File.Create(cacheFile)); + return BinaryTranslator.GetWriteTranslator(stream); } else { - return BinaryTranslator.GetReadTranslator(File.OpenRead(cacheFile), InterningBinaryReader.PoolingBuffer); + return BinaryTranslator.GetReadTranslator(stream, InterningBinaryReader.PoolingBuffer); } } diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index b543973746e..093ad6113bd 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -1201,49 +1201,51 @@ internal static void KillTree(int processIdToKill) // Grab the process handle. We want to keep this open for the duration of the function so that // it cannot be reused while we are running. - SafeProcessHandle hProcess = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, processIdToKill); - if (hProcess.IsInvalid) + using (SafeProcessHandle hProcess = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, processIdToKill)) { - return; - } - - try - { - try - { - // Kill this process, so that no further children can be created. - thisProcess.Kill(); - } - catch (Win32Exception e) when (e.NativeErrorCode == ERROR_ACCESS_DENIED) + if (hProcess.IsInvalid) { - // Access denied is potentially expected -- it happens when the process that - // we're attempting to kill is already dead. So just ignore in that case. + return; } - // Now enumerate our children. Children of this process are any process which has this process id as its parent - // and which also started after this process did. - List> children = GetChildProcessIds(processIdToKill, myStartTime); - try { - foreach (KeyValuePair childProcessInfo in children) + try + { + // Kill this process, so that no further children can be created. + thisProcess.Kill(); + } + catch (Win32Exception e) when (e.NativeErrorCode == ERROR_ACCESS_DENIED) { - KillTree(childProcessInfo.Key); + // Access denied is potentially expected -- it happens when the process that + // we're attempting to kill is already dead. So just ignore in that case. + } + + // Now enumerate our children. Children of this process are any process which has this process id as its parent + // and which also started after this process did. + List> children = GetChildProcessIds(processIdToKill, myStartTime); + + try + { + foreach (KeyValuePair childProcessInfo in children) + { + KillTree(childProcessInfo.Key); + } + } + finally + { + foreach (KeyValuePair childProcessInfo in children) + { + childProcessInfo.Value.Dispose(); + } } } finally { - foreach (KeyValuePair childProcessInfo in children) - { - childProcessInfo.Value.Dispose(); - } + // Release the handle. After this point no more children of this process exist and this process has also exited. + hProcess.Dispose(); } } - finally - { - // Release the handle. After this point no more children of this process exist and this process has also exited. - hProcess.Dispose(); - } } finally { @@ -1296,26 +1298,27 @@ internal static int GetParentProcessId(int processId) else #endif { - SafeProcessHandle hProcess = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, processId); - - if (!hProcess.IsInvalid) + using SafeProcessHandle hProcess = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, processId); { - try + if (!hProcess.IsInvalid) { - // UNDONE: NtQueryInformationProcess will fail if we are not elevated and other process is. Advice is to change to use ToolHelp32 API's - // For now just return zero and worst case we will not kill some children. - PROCESS_BASIC_INFORMATION pbi = new PROCESS_BASIC_INFORMATION(); - int pSize = 0; + try + { + // UNDONE: NtQueryInformationProcess will fail if we are not elevated and other process is. Advice is to change to use ToolHelp32 API's + // For now just return zero and worst case we will not kill some children. + PROCESS_BASIC_INFORMATION pbi = new PROCESS_BASIC_INFORMATION(); + int pSize = 0; - if (0 == NtQueryInformationProcess(hProcess, PROCESSINFOCLASS.ProcessBasicInformation, ref pbi, pbi.Size, ref pSize)) + if (0 == NtQueryInformationProcess(hProcess, PROCESSINFOCLASS.ProcessBasicInformation, ref pbi, pbi.Size, ref pSize)) + { + ParentID = (int)pbi.InheritedFromUniqueProcessId; + } + } + finally { - ParentID = (int)pbi.InheritedFromUniqueProcessId; + hProcess.Dispose(); } } - finally - { - hProcess.Dispose(); - } } } @@ -1337,34 +1340,36 @@ internal static List> GetChildProcessIds(in { // Hold the child process handle open so that children cannot die and restart with a different parent after we've started looking at it. // This way, any handle we pass back is guaranteed to be one of our actual children. - SafeProcessHandle childHandle = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, possibleChildProcess.Id); - if (childHandle.IsInvalid) + using (SafeProcessHandle childHandle = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, possibleChildProcess.Id)) { - continue; - } + if (childHandle.IsInvalid) + { + continue; + } - bool keepHandle = false; - try - { - if (possibleChildProcess.StartTime > parentStartTime) + bool keepHandle = false; + try { - int childParentProcessId = GetParentProcessId(possibleChildProcess.Id); - if (childParentProcessId != 0) + if (possibleChildProcess.StartTime > parentStartTime) { - if (parentProcessId == childParentProcessId) + int childParentProcessId = GetParentProcessId(possibleChildProcess.Id); + if (childParentProcessId != 0) { - // Add this one - myChildren.Add(new KeyValuePair(possibleChildProcess.Id, childHandle)); - keepHandle = true; + if (parentProcessId == childParentProcessId) + { + // Add this one + myChildren.Add(new KeyValuePair(possibleChildProcess.Id, childHandle)); + keepHandle = true; + } } } } - } - finally - { - if (!keepHandle) + finally { - childHandle.Dispose(); + if (!keepHandle) + { + childHandle.Dispose(); + } } } } diff --git a/src/MSBuild/XMake.cs b/src/MSBuild/XMake.cs index 213b842ae6a..8aa95e9ab23 100644 --- a/src/MSBuild/XMake.cs +++ b/src/MSBuild/XMake.cs @@ -662,6 +662,9 @@ public static ExitType Execute( ExitType exitType = ExitType.Success; ConsoleCancelEventHandler cancelHandler = Console_CancelKeyPress; + + TextWriter preprocessWriter = null; + TextWriter targetsWriter = null; try { #if FEATURE_GET_COMMANDLINE @@ -701,8 +704,6 @@ public static ExitType Execute( #else bool enableNodeReuse = false; #endif - TextWriter preprocessWriter = null; - TextWriter targetsWriter = null; bool detailedSummary = false; ISet warningsAsErrors = null; ISet warningsNotAsErrors = null; @@ -819,12 +820,15 @@ public static ExitType Execute( } else if ((getProperty.Length > 0 || getItem.Length > 0) && (targets is null || targets.Length == 0)) { + TextWriter output = null; try { using (ProjectCollection collection = new(globalProperties, loggers, ToolsetDefinitionLocations.Default)) { Project project = collection.LoadProject(projectFile, globalProperties, toolsVersion); - TextWriter output = getResultOutputFile.Length > 0 ? new StreamWriter(getResultOutputFile) : Console.Out; + output = getResultOutputFile.Length > 0 + ? new StreamWriter(getResultOutputFile) + : Console.Out; exitType = OutputPropertiesAfterEvaluation(getProperty, getItem, project, output); collection.LogBuildFinishedEvent(exitType == ExitType.Success); } @@ -833,6 +837,10 @@ public static ExitType Execute( { exitType = ExitType.BuildError; } + finally + { + output?.Dispose(); + } } else // regular build { @@ -885,10 +893,18 @@ public static ExitType Execute( string timerOutputFilename = Environment.GetEnvironmentVariable("MSBUILDTIMEROUTPUTS"); - if (outputPropertiesItemsOrTargetResults && targets?.Length > 0 && result is not null) + TextWriter outputStream = null; + try + { + if (outputPropertiesItemsOrTargetResults && targets?.Length > 0 && result is not null) + { + outputStream = getResultOutputFile.Length > 0 ? new StreamWriter(getResultOutputFile) : Console.Out; + exitType = OutputBuildInformationInJson(result, getProperty, getItem, getTargetResult, loggers, exitType, outputStream); + } + } + finally { - TextWriter outputStream = getResultOutputFile.Length > 0 ? new StreamWriter(getResultOutputFile) : Console.Out; - exitType = OutputBuildInformationInJson(result, getProperty, getItem, getTargetResult, loggers, exitType, outputStream); + outputStream?.Dispose(); } if (!string.IsNullOrEmpty(timerOutputFilename)) @@ -1032,6 +1048,9 @@ public static ExitType Execute( NativeMethodsShared.RestoreConsoleMode(s_originalConsoleMode); + preprocessWriter?.Dispose(); + targetsWriter?.Dispose(); + #if FEATURE_GET_COMMANDLINE MSBuildEventSource.Log.MSBuildExeStop(commandLine); #else @@ -3764,9 +3783,9 @@ private static ILogger[] ProcessLoggingSwitches( ProcessConsoleLoggerSwitch(noConsoleLogger, consoleLoggerParameters, distributedLoggerRecords, verbosity, cpuCount, loggers); } - ProcessDistributedFileLogger(distributedFileLogger, fileLoggerParameters, distributedLoggerRecords, loggers, cpuCount); + ProcessDistributedFileLogger(distributedFileLogger, fileLoggerParameters, distributedLoggerRecords); - ProcessFileLoggers(groupedFileLoggerParameters, distributedLoggerRecords, verbosity, cpuCount, loggers); + ProcessFileLoggers(groupedFileLoggerParameters, distributedLoggerRecords, cpuCount, loggers); verbosity = outVerbosity; @@ -3808,7 +3827,7 @@ internal static string AggregateParameters(string anyPrefixingParameter, string[ /// Add a file logger with the appropriate parameters to the loggers list for each /// non-empty set of file logger parameters provided. /// - private static void ProcessFileLoggers(string[][] groupedFileLoggerParameters, List distributedLoggerRecords, LoggerVerbosity verbosity, int cpuCount, List loggers) + private static void ProcessFileLoggers(string[][] groupedFileLoggerParameters, List distributedLoggerRecords, int cpuCount, List loggers) { for (int i = 0; i < groupedFileLoggerParameters.Length; i++) { @@ -4001,9 +4020,7 @@ private static DistributedLoggerRecord CreateForwardingLoggerRecord(ILogger logg internal static void ProcessDistributedFileLogger( bool distributedFileLogger, string[] fileLoggerParameters, - List distributedLoggerRecords, - List loggers, - int cpuCount) + List distributedLoggerRecords) { if (distributedFileLogger) { From a372cb7628d7aa4615a6ec490b790ef573799886 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Tue, 9 Apr 2024 20:04:39 +0200 Subject: [PATCH 02/20] remove extra using --- src/Build/BackEnd/Shared/TargetResult.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Build/BackEnd/Shared/TargetResult.cs b/src/Build/BackEnd/Shared/TargetResult.cs index b490f005543..02f8d92045d 100644 --- a/src/Build/BackEnd/Shared/TargetResult.cs +++ b/src/Build/BackEnd/Shared/TargetResult.cs @@ -10,7 +10,6 @@ using Microsoft.Build.Framework; using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; -using static BuildXL.Processes.SandboxConnectionKext; using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem; #nullable disable From 9884345256307e25c6fd811e5e69e4d6fce6c7e6 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Wed, 10 Apr 2024 11:17:13 +0200 Subject: [PATCH 03/20] remove extra params --- src/MSBuild.UnitTests/XMake_Tests.cs | 52 +++++++--------------------- 1 file changed, 13 insertions(+), 39 deletions(-) diff --git a/src/MSBuild.UnitTests/XMake_Tests.cs b/src/MSBuild.UnitTests/XMake_Tests.cs index 1c3aed6df74..8691917bc02 100644 --- a/src/MSBuild.UnitTests/XMake_Tests.cs +++ b/src/MSBuild.UnitTests/XMake_Tests.cs @@ -1989,9 +1989,7 @@ public void TestProcessFileLoggerSwitch1() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); distributedLoggerRecords.Count.ShouldBe(0); // "Expected no distributed loggers to be attached" loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" } @@ -2010,9 +2008,7 @@ public void TestProcessFileLoggerSwitch2() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); distributedLoggerRecords.Count.ShouldBe(1); // "Expected one distributed loggers to be attached" loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" } @@ -2031,9 +2027,7 @@ public void TestProcessFileLoggerSwitch3() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); distributedLoggerRecords.Count.ShouldBe(0); // "Expected no distributed loggers to be attached" loggers.Count.ShouldBe(0); // "Expected a central loggers to be attached" @@ -2045,9 +2039,7 @@ public void TestProcessFileLoggerSwitch3() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); distributedLoggerRecords.Count.ShouldBe(0); // "Expected no distributed loggers to be attached" loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" @@ -2058,9 +2050,7 @@ public void TestProcessFileLoggerSwitch3() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); distributedLoggerRecords.Count.ShouldBe(0); // "Expected no distributed loggers to be attached" loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" } @@ -2079,9 +2069,7 @@ public void TestProcessFileLoggerSwitch4() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" distributedLoggerRecords.Count.ShouldBe(1); // "Expected a distributed logger to be attached" distributedLoggerRecords[0].ForwardingLoggerDescription.LoggerSwitchParameters.ShouldBe($"logFile={Path.Combine(Directory.GetCurrentDirectory(), "MSBuild.log")}", StringCompareShould.IgnoreCase); // "Expected parameter in logger to match parameter passed in" @@ -2094,9 +2082,7 @@ public void TestProcessFileLoggerSwitch4() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" distributedLoggerRecords.Count.ShouldBe(1); // "Expected a distributed logger to be attached" distributedLoggerRecords[0].ForwardingLoggerDescription.LoggerSwitchParameters.ShouldBe($"{fileLoggerParameters[0]};logFile={Path.Combine(Directory.GetCurrentDirectory(), "MSBuild.log")}", StringCompareShould.IgnoreCase); // "Expected parameter in logger to match parameter passed in" @@ -2109,9 +2095,7 @@ public void TestProcessFileLoggerSwitch4() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" distributedLoggerRecords.Count.ShouldBe(1); // "Expected a distributed logger to be attached" distributedLoggerRecords[0].ForwardingLoggerDescription.LoggerSwitchParameters.ShouldBe($"{fileLoggerParameters[0]};logFile={Path.Combine(Directory.GetCurrentDirectory(), "MSBuild.log")}", StringCompareShould.IgnoreCase); // "Expected parameter in logger to match parameter passed in" @@ -2124,9 +2108,7 @@ public void TestProcessFileLoggerSwitch4() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" distributedLoggerRecords.Count.ShouldBe(1); // "Expected a distributed logger to be attached" distributedLoggerRecords[0].ForwardingLoggerDescription.LoggerSwitchParameters.ShouldBe($";Parameter1;logFile={Path.Combine(Directory.GetCurrentDirectory(), "MSBuild.log")}", StringCompareShould.IgnoreCase); // "Expected parameter in logger to match parameter passed in" @@ -2139,9 +2121,7 @@ public void TestProcessFileLoggerSwitch4() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" distributedLoggerRecords.Count.ShouldBe(1); // "Expected a distributed logger to be attached" distributedLoggerRecords[0].ForwardingLoggerDescription.LoggerSwitchParameters.ShouldBe(fileLoggerParameters[0] + ";" + fileLoggerParameters[1], StringCompareShould.IgnoreCase); // "Expected parameter in logger to match parameter passed in" @@ -2152,9 +2132,7 @@ public void TestProcessFileLoggerSwitch4() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" distributedLoggerRecords.Count.ShouldBe(1); // "Expected a distributed logger to be attached" distributedLoggerRecords[0].ForwardingLoggerDescription.LoggerSwitchParameters.ShouldBe($"Parameter1;verbosity=Normal;logFile={Path.Combine(Directory.GetCurrentDirectory(), "..", "cat.log")};Parameter1", StringCompareShould.IgnoreCase); // "Expected parameter in logger to match parameter passed in" @@ -2165,9 +2143,7 @@ public void TestProcessFileLoggerSwitch4() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 2); + distributedLoggerRecords); distributedLoggerRecords[0].ForwardingLoggerDescription.LoggerSwitchParameters.ShouldBe($"Parameter1;Parameter;;;Parameter;Parameter;logFile={Path.Combine(Directory.GetCurrentDirectory(), "msbuild.log")}", StringCompareShould.IgnoreCase); // "Expected parameter in logger to match parameter passed in" } @@ -2185,9 +2161,7 @@ public void TestProcessFileLoggerSwitch5() MSBuildApp.ProcessDistributedFileLogger( distributedFileLogger, fileLoggerParameters, - distributedLoggerRecords, - loggers, - 1); + distributedLoggerRecords); distributedLoggerRecords.Count.ShouldBe(0); // "Expected no distributed loggers to be attached" loggers.Count.ShouldBe(0); // "Expected no central loggers to be attached" } From 8a0f3706705233b2ab9855ce3803ddc6619f97d0 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Wed, 10 Apr 2024 11:56:34 +0200 Subject: [PATCH 04/20] add dispose part 2 --- .../BuildManager/LegacyThreadingData.cs | 6 ++++ src/Build/BackEnd/Node/OutOfProcServerNode.cs | 15 ++++---- src/Build/Evaluation/IntrinsicFunctions.cs | 35 ++++++++++++------- .../LazyItemEvaluator.IncludeOperation.cs | 26 +++++++++----- src/Build/Instance/ProjectInstance.cs | 28 ++++++++++----- .../BinaryLogger/BuildEventArgsReader.cs | 7 ++-- .../Postprocessing/StreamExtensions.cs | 4 ++- src/Shared/NodeEndpointOutOfProcBase.cs | 21 +++++++---- 8 files changed, 93 insertions(+), 49 deletions(-) diff --git a/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs b/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs index 5670a61e3f1..ed9940d054d 100644 --- a/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs +++ b/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; using Microsoft.Build.BackEnd; @@ -82,6 +83,7 @@ internal int MainThreadSubmissionId /// Given a submission ID, assign it "start" and "finish" events to track its use of /// the legacy thread. /// + [SuppressMessage("Microsoft.Naming", "CA2000:Dispose objects before losing scope", Justification = "The events are disposed in UnregisterSubmissionForLegacyThread")] internal void RegisterSubmissionForLegacyThread(int submissionId) { lock (_legacyThreadingEventsLock) @@ -104,6 +106,10 @@ internal void UnregisterSubmissionForLegacyThread(int submissionId) { ErrorUtilities.VerifyThrow(_legacyThreadingEventsById.ContainsKey(submissionId), "Submission {0} should have been previously registered with LegacyThreadingData", submissionId); + // Dispose the events + _legacyThreadingEventsById[submissionId].Item1?.Dispose(); + _legacyThreadingEventsById[submissionId].Item2?.Dispose(); + _legacyThreadingEventsById.Remove(submissionId); } } diff --git a/src/Build/BackEnd/Node/OutOfProcServerNode.cs b/src/Build/BackEnd/Node/OutOfProcServerNode.cs index 54a45f77816..42c8d8540cb 100644 --- a/src/Build/BackEnd/Node/OutOfProcServerNode.cs +++ b/src/Build/BackEnd/Node/OutOfProcServerNode.cs @@ -406,8 +406,10 @@ private void HandleServerNodeBuildCommand(ServerNodeBuildCommand command) (int exitCode, string exitType) buildResult; // Dispose must be called before the server sends ServerNodeBuildResult packet - using (var outWriter = RedirectConsoleWriter.Create(text => SendPacket(new ServerNodeConsoleWrite(text, ConsoleOutput.Standard)))) - using (var errWriter = RedirectConsoleWriter.Create(text => SendPacket(new ServerNodeConsoleWrite(text, ConsoleOutput.Error)))) + using (var outRedirectWriter = new RedirectConsoleWriter(text => SendPacket(new ServerNodeConsoleWrite(text, ConsoleOutput.Standard)))) + using (var errRedirectWriter = new RedirectConsoleWriter(text => SendPacket(new ServerNodeConsoleWrite(text, ConsoleOutput.Error)))) + using (var outWriter = outRedirectWriter.SyncWriter) + using (var errWriter = errRedirectWriter.SyncWriter) { Console.SetOut(outWriter); Console.SetError(errWriter); @@ -437,19 +439,14 @@ internal sealed class RedirectConsoleWriter : StringWriter private readonly Timer _timer; private readonly TextWriter _syncWriter; - private RedirectConsoleWriter(Action writeCallback) + internal RedirectConsoleWriter(Action writeCallback) { _writeCallback = writeCallback; _syncWriter = Synchronized(this); _timer = new Timer(TimerCallback, null, 0, 40); } - public static TextWriter Create(Action writeCallback) - { - RedirectConsoleWriter writer = new(writeCallback); - - return writer._syncWriter; - } + internal TextWriter SyncWriter => _syncWriter; private void TimerCallback(object? state) { diff --git a/src/Build/Evaluation/IntrinsicFunctions.cs b/src/Build/Evaluation/IntrinsicFunctions.cs index 6f8c5ed00f6..84aefed175a 100644 --- a/src/Build/Evaluation/IntrinsicFunctions.cs +++ b/src/Build/Evaluation/IntrinsicFunctions.cs @@ -291,26 +291,34 @@ internal static object GetRegistryValueFromView(string keyName, string valueName return string.Empty; } - using (RegistryKey key = GetBaseKeyFromKeyName(keyName, view, out string subKeyName)) + RegistryKey key = null; + try { - if (key != null) + using (key = GetBaseKeyFromKeyName(keyName, view, out string subKeyName)) { - using (RegistryKey subKey = key.OpenSubKey(subKeyName, false)) + if (key != null) { - // If we managed to retrieve the subkey, then move onto locating the value - if (subKey != null) + using (RegistryKey subKey = key.OpenSubKey(subKeyName, false)) { - result = subKey.GetValue(valueName); - } - - // We've found a value, so stop looking - if (result != null) - { - break; + // If we managed to retrieve the subkey, then move onto locating the value + if (subKey != null) + { + result = subKey.GetValue(valueName); + } + + // We've found a value, so stop looking + if (result != null) + { + break; + } } } } } + finally + { + key?.Dispose(); + } } } @@ -446,12 +454,13 @@ internal static object StableStringHash(string toHash, StringHashingAlgorithm al private static string CalculateSha256(string toHash) { - var sha = System.Security.Cryptography.SHA256.Create(); + using var sha = System.Security.Cryptography.SHA256.Create(); var hashResult = new StringBuilder(); foreach (byte theByte in sha.ComputeHash(Encoding.UTF8.GetBytes(toHash))) { hashResult.Append(theByte.ToString("x2")); } + return hashResult.ToString(); } diff --git a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs index 9dfd281b165..fc58a03146a 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs @@ -107,16 +107,24 @@ protected override ImmutableArray SelectItems(OrderedItemDataCollection.Build MSBuildEventSource.Log.ExpandGlobStart(_rootDirectory ?? string.Empty, glob, string.Join(", ", excludePatternsForGlobs)); } - using (_lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs)) + IDisposable? disposableGlob = null; + try { - includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped( - _rootDirectory, - glob, - excludePatternsForGlobs, - fileMatcher: FileMatcher, - loggingMechanism: _lazyEvaluator._loggingContext, - includeLocation: _itemElement.IncludeLocation, - excludeLocation: _itemElement.ExcludeLocation); + using (disposableGlob = _lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs)) + { + includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped( + _rootDirectory, + glob, + excludePatternsForGlobs, + fileMatcher: FileMatcher, + loggingMechanism: _lazyEvaluator._loggingContext, + includeLocation: _itemElement.IncludeLocation, + excludeLocation: _itemElement.ExcludeLocation); + } + } + finally + { + disposableGlob?.Dispose(); } if (MSBuildEventSource.Log.IsEnabled()) diff --git a/src/Build/Instance/ProjectInstance.cs b/src/Build/Instance/ProjectInstance.cs index fe63676c1d2..5f14cd1dd0d 100644 --- a/src/Build/Instance/ProjectInstance.cs +++ b/src/Build/Instance/ProjectInstance.cs @@ -2768,14 +2768,26 @@ private static ProjectInstance[] GenerateSolutionWrapperUsingOldOM( } } - XmlReaderSettings xrs = new XmlReaderSettings(); - xrs.DtdProcessing = DtdProcessing.Ignore; - - ProjectRootElement projectRootElement = new ProjectRootElement(XmlReader.Create(new StringReader(wrapperProjectXml), xrs), projectRootElementCache, isExplicitlyLoaded, - preserveFormatting: false); - projectRootElement.DirectoryPath = Path.GetDirectoryName(projectFile); - ProjectInstance instance = new ProjectInstance(projectRootElement, globalProperties, toolsVersion, buildParameters, loggingService, projectBuildEventContext, sdkResolverService, submissionId); - return new ProjectInstance[] { instance }; + XmlReaderSettings xrs = new XmlReaderSettings + { + DtdProcessing = DtdProcessing.Ignore + }; + + StringReader sr = new StringReader(wrapperProjectXml); + using (XmlReader xmlReader = XmlReader.Create(sr, xrs)) + { + ProjectRootElement projectRootElement = new( + xmlReader, + projectRootElementCache, + isExplicitlyLoaded, + preserveFormatting: false) + { + DirectoryPath = Path.GetDirectoryName(projectFile) + }; + ProjectInstance instance = new(projectRootElement, globalProperties, toolsVersion, buildParameters, loggingService, projectBuildEventContext, sdkResolverService, submissionId); + + return [instance]; + } } /// diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs index 2c49c17c8a7..720ef5b0fd1 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs @@ -185,9 +185,10 @@ internal RawRecord ReadRaw() int serializedEventLength = ReadInt32(); Stream stream = _binaryReader.BaseStream.Slice(serializedEventLength); - _lastSubStream = stream as SubStream; - - _recordNumber += 1; + using (_lastSubStream = stream as SubStream) + { + _recordNumber += 1; + } return new(recordKind, stream); } diff --git a/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs b/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs index 2993b3953c1..93bb8382958 100644 --- a/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs +++ b/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs @@ -60,7 +60,9 @@ public static byte[] ReadToEnd(this Stream stream) { if (stream.TryGetLength(out long length)) { - BinaryReader reader = new(stream); + //check with Jan + using BinaryReader reader = new(stream); + return reader.ReadBytes((int)length); } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index 6c12448b1ad..e2badae5526 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -481,10 +481,15 @@ private void PacketPumpProc() } } - RunReadLoop( - new BufferedReadStream(_pipeServer), - _pipeServer, - localPacketQueue, localPacketAvailable, localTerminatePacketPump); + using (var localReadPipe = new BufferedReadStream(_pipeServer)) + { + RunReadLoop( + localReadPipe, + _pipeServer, + localPacketQueue, + localPacketAvailable, + localTerminatePacketPump); + } CommunicationsUtilities.Trace("Ending read loop"); @@ -508,8 +513,12 @@ private void PacketPumpProc() } } - private void RunReadLoop(Stream localReadPipe, Stream localWritePipe, - ConcurrentQueue localPacketQueue, AutoResetEvent localPacketAvailable, AutoResetEvent localTerminatePacketPump) + private void RunReadLoop( + Stream localReadPipe, + Stream localWritePipe, + ConcurrentQueue localPacketQueue, + AutoResetEvent localPacketAvailable, + AutoResetEvent localTerminatePacketPump) { // Ordering of the wait handles is important. The first signalled wait handle in the array // will be returned by WaitAny if multiple wait handles are signalled. We prefer to have the From e011c020a48f368ca0497bfadff1dcdfa95c0e98 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Wed, 10 Apr 2024 12:08:11 +0200 Subject: [PATCH 05/20] fix styling errors --- src/Build/Instance/ProjectInstance.cs | 2 +- .../Logging/BinaryLogger/Postprocessing/StreamExtensions.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build/Instance/ProjectInstance.cs b/src/Build/Instance/ProjectInstance.cs index 5f14cd1dd0d..46a4ac1785d 100644 --- a/src/Build/Instance/ProjectInstance.cs +++ b/src/Build/Instance/ProjectInstance.cs @@ -2786,7 +2786,7 @@ private static ProjectInstance[] GenerateSolutionWrapperUsingOldOM( }; ProjectInstance instance = new(projectRootElement, globalProperties, toolsVersion, buildParameters, loggingService, projectBuildEventContext, sdkResolverService, submissionId); - return [instance]; + return new[] { instance }; } } diff --git a/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs b/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs index 93bb8382958..7f2114819a3 100644 --- a/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs +++ b/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs @@ -60,7 +60,7 @@ public static byte[] ReadToEnd(this Stream stream) { if (stream.TryGetLength(out long length)) { - //check with Jan + // check with Jan using BinaryReader reader = new(stream); return reader.ReadBytes((int)length); From 202049e415d06f2560671ead8fb9061a4a373eed Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Wed, 10 Apr 2024 12:19:59 +0200 Subject: [PATCH 06/20] fix test --- src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs b/src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs index 0e83218dce7..6eb915775be 100644 --- a/src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs +++ b/src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs @@ -18,7 +18,8 @@ public async Task EmitConsoleMessages() { StringBuilder sb = new StringBuilder(); - using (TextWriter writer = OutOfProcServerNode.RedirectConsoleWriter.Create(text => sb.Append(text))) + using (var rw = new OutOfProcServerNode.RedirectConsoleWriter(text => sb.Append(text))) + using (var writer = rw.SyncWriter) { writer.WriteLine("Line 1"); await Task.Delay(80); // should be somehow bigger than `RedirectConsoleWriter` flush period - see its constructor From 6a0cca6dd5c1d774b1c190d567636510c6e9c5d3 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Wed, 10 Apr 2024 12:47:16 +0200 Subject: [PATCH 07/20] remove invalid dispose --- .../LazyItemEvaluator.IncludeOperation.cs | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs index fc58a03146a..d40ea3145fa 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; using System.Linq; using Microsoft.Build.Construction; using Microsoft.Build.Eventing; @@ -33,6 +34,7 @@ public IncludeOperation(IncludeOperationBuilder builder, LazyItemEvaluator SelectItems(OrderedItemDataCollection.Builder listBuilder, ImmutableHashSet globsToIgnore) { ImmutableArray.Builder? itemsToAdd = null; @@ -107,24 +109,16 @@ protected override ImmutableArray SelectItems(OrderedItemDataCollection.Build MSBuildEventSource.Log.ExpandGlobStart(_rootDirectory ?? string.Empty, glob, string.Join(", ", excludePatternsForGlobs)); } - IDisposable? disposableGlob = null; - try + using (_lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs)) { - using (disposableGlob = _lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs)) - { - includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped( - _rootDirectory, - glob, - excludePatternsForGlobs, - fileMatcher: FileMatcher, - loggingMechanism: _lazyEvaluator._loggingContext, - includeLocation: _itemElement.IncludeLocation, - excludeLocation: _itemElement.ExcludeLocation); - } - } - finally - { - disposableGlob?.Dispose(); + includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped( + _rootDirectory, + glob, + excludePatternsForGlobs, + fileMatcher: FileMatcher, + loggingMechanism: _lazyEvaluator._loggingContext, + includeLocation: _itemElement.IncludeLocation, + excludeLocation: _itemElement.ExcludeLocation); } if (MSBuildEventSource.Log.IsEnabled()) From 6b56301bafbabe9082d9aaac311b8960895f46aa Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Thu, 11 Apr 2024 10:05:32 +0200 Subject: [PATCH 08/20] dispose part 3 --- .../BuildManager/LegacyThreadingData.cs | 2 +- .../Engine/Engine/IntrinsicFunctions.cs | 32 ++++++----- src/Shared/NodeEndpointOutOfProcBase.cs | 21 +++----- src/Tasks/AppConfig/AppConfig.cs | 6 ++- .../BootstrapperUtil/BootstrapperBuilder.cs | 12 +++-- src/Tasks/CodeTaskFactory.cs | 2 +- src/Tasks/DownloadFile.cs | 3 +- src/Tasks/GenerateResource.cs | 54 +++++++++++++------ src/Tasks/GetAssembliesMetadata.cs | 2 +- src/Tasks/GetInstalledSDKLocations.cs | 2 +- src/Tasks/ManifestUtil/ManifestFormatter.cs | 4 +- src/Tasks/ManifestUtil/ManifestReader.cs | 2 +- src/Tasks/ManifestUtil/ManifestWriter.cs | 2 +- src/Tasks/ManifestUtil/SecurityUtil.cs | 6 +-- src/Tasks/ManifestUtil/TrustInfo.cs | 3 +- src/Tasks/ManifestUtil/Util.cs | 40 ++++++++------ src/Tasks/ManifestUtil/XmlUtil.cs | 2 +- src/Tasks/ManifestUtil/mansign2.cs | 3 +- src/Tasks/ResGen.cs | 9 ++-- src/Tasks/ResolveKeySource.cs | 2 +- .../ResourceHandling/FileStreamResource.cs | 3 ++ src/Tasks/Unzip.cs | 9 +++- src/Tasks/WriteCodeFragment.cs | 6 ++- src/Tasks/XamlTaskFactory/TaskParser.cs | 8 ++- src/Tasks/XamlTaskFactory/XamlTaskFactory.cs | 2 +- src/Tasks/XslTransformation.cs | 8 ++- src/UnitTests.Shared/ObjectModelHelpers.cs | 3 +- 27 files changed, 153 insertions(+), 95 deletions(-) diff --git a/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs b/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs index ed9940d054d..33dc9fe2cc4 100644 --- a/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs +++ b/src/Build/BackEnd/BuildManager/LegacyThreadingData.cs @@ -83,7 +83,7 @@ internal int MainThreadSubmissionId /// Given a submission ID, assign it "start" and "finish" events to track its use of /// the legacy thread. /// - [SuppressMessage("Microsoft.Naming", "CA2000:Dispose objects before losing scope", Justification = "The events are disposed in UnregisterSubmissionForLegacyThread")] + [SuppressMessage("Microsoft.Dispose", "CA2000:Dispose objects before losing scope", Justification = "The events are disposed in UnregisterSubmissionForLegacyThread")] internal void RegisterSubmissionForLegacyThread(int submissionId) { lock (_legacyThreadingEventsLock) diff --git a/src/Deprecated/Engine/Engine/IntrinsicFunctions.cs b/src/Deprecated/Engine/Engine/IntrinsicFunctions.cs index e38ed53576e..340e769371f 100644 --- a/src/Deprecated/Engine/Engine/IntrinsicFunctions.cs +++ b/src/Deprecated/Engine/Engine/IntrinsicFunctions.cs @@ -197,26 +197,34 @@ internal static object GetRegistryValueFromView(string keyName, string valueName // of that error. RegistryView view = (RegistryView)Enum.Parse(typeof(RegistryView), viewAsString, true); - using (RegistryKey key = GetBaseKeyFromKeyName(keyName, view, out subKeyName)) + RegistryKey key = null; + try { - if (key != null) + using (key = GetBaseKeyFromKeyName(keyName, view, out subKeyName)) { - using (RegistryKey subKey = key.OpenSubKey(subKeyName, false)) + if (key != null) { - // If we managed to retrieve the subkey, then move onto locating the value - if (subKey != null) + using (RegistryKey subKey = key.OpenSubKey(subKeyName, false)) { - result = subKey.GetValue(valueName); - } - - // We've found a value, so stop looking - if (result != null) - { - break; + // If we managed to retrieve the subkey, then move onto locating the value + if (subKey != null) + { + result = subKey.GetValue(valueName); + } + + // We've found a value, so stop looking + if (result != null) + { + break; + } } } } } + finally + { + key?.Dispose(); + } } } diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index e2badae5526..6c12448b1ad 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -481,15 +481,10 @@ private void PacketPumpProc() } } - using (var localReadPipe = new BufferedReadStream(_pipeServer)) - { - RunReadLoop( - localReadPipe, - _pipeServer, - localPacketQueue, - localPacketAvailable, - localTerminatePacketPump); - } + RunReadLoop( + new BufferedReadStream(_pipeServer), + _pipeServer, + localPacketQueue, localPacketAvailable, localTerminatePacketPump); CommunicationsUtilities.Trace("Ending read loop"); @@ -513,12 +508,8 @@ private void PacketPumpProc() } } - private void RunReadLoop( - Stream localReadPipe, - Stream localWritePipe, - ConcurrentQueue localPacketQueue, - AutoResetEvent localPacketAvailable, - AutoResetEvent localTerminatePacketPump) + private void RunReadLoop(Stream localReadPipe, Stream localWritePipe, + ConcurrentQueue localPacketQueue, AutoResetEvent localPacketAvailable, AutoResetEvent localTerminatePacketPump) { // Ordering of the wait handles is important. The first signalled wait handle in the array // will be returned by WaitAny if multiple wait handles are signalled. We prefer to have the diff --git a/src/Tasks/AppConfig/AppConfig.cs b/src/Tasks/AppConfig/AppConfig.cs index 7f50e75cc29..40932d287a5 100644 --- a/src/Tasks/AppConfig/AppConfig.cs +++ b/src/Tasks/AppConfig/AppConfig.cs @@ -34,8 +34,10 @@ internal void Load(string appConfigFilePath) // Need a filestream as the XmlReader doesn't support nonstandard unicode characters in path. // No need to dispose - as 'CloseInput' was passed to XmlReaderSettings FileStream fs = File.OpenRead(appConfigFilePath); - reader = XmlReader.Create(fs, readerSettings); - Read(reader); + using (reader = XmlReader.Create(fs, readerSettings)) + { + Read(reader); + } } catch (XmlException e) { diff --git a/src/Tasks/BootstrapperUtil/BootstrapperBuilder.cs b/src/Tasks/BootstrapperUtil/BootstrapperBuilder.cs index 0b296e958ca..76e17988b08 100644 --- a/src/Tasks/BootstrapperUtil/BootstrapperBuilder.cs +++ b/src/Tasks/BootstrapperUtil/BootstrapperBuilder.cs @@ -225,7 +225,8 @@ public BuildResults Build(BuildSettings settings) var fi = new FileInfo(de.Value); using (FileStream fs = fi.OpenRead()) { - data = new StreamReader(fs).ReadToEnd(); + using var sr = new StreamReader(fs); + data = sr.ReadToEnd(); } resourceUpdater.AddStringResource(44, de.Key, data); @@ -835,7 +836,7 @@ private XmlDocument LoadAndValidateXmlDocument(string filePath, bool validateFil if (validate) { #pragma warning disable 618 // Using XmlValidatingReader. TODO: We need to switch to using XmlReader.Create() with validation. - var validatingReader = new XmlValidatingReader(xmlReader); + using var validatingReader = new XmlValidatingReader(xmlReader); #pragma warning restore 618 var xrSettings = new XmlReaderSettings { DtdProcessing = DtdProcessing.Ignore, CloseInput = true }; FileStream fs = File.OpenRead(schemaPath); @@ -1657,7 +1658,7 @@ private static string GetFileHash(string filePath) // pre-signed anwyay; this is a fallback in case we ever encounter a bootstrapper that is // not signed. #pragma warning disable SA1111, SA1009 // Closing parenthesis should be on line of last parameter - System.Security.Cryptography.SHA256 sha = System.Security.Cryptography.SHA256.Create( + using System.Security.Cryptography.SHA256 sha = System.Security.Cryptography.SHA256.Create( #if FEATURE_CRYPTOGRAPHIC_FACTORY_ALGORITHM_NAMES "System.Security.Cryptography.SHA256CryptoServiceProvider" #endif @@ -2033,7 +2034,8 @@ private static void DumpXmlToFile(XmlNode node, string fileName) { xmlwriter.Formatting = Formatting.Indented; xmlwriter.Indentation = 4; - xmlwriter.WriteNode(new XmlNodeReader(node), true); + using var xmlReader = new XmlNodeReader(node); + xmlwriter.WriteNode(xmlReader, true); } } catch (IOException) @@ -2194,7 +2196,7 @@ private static string GetPublicKeyOfFile(string fileSource) { try { - var cert = new X509Certificate(fileSource); + using var cert = new X509Certificate(fileSource); string publicKey = cert.GetPublicKeyString(); return publicKey; } diff --git a/src/Tasks/CodeTaskFactory.cs b/src/Tasks/CodeTaskFactory.cs index f863c969cbf..e1923c87f9d 100644 --- a/src/Tasks/CodeTaskFactory.cs +++ b/src/Tasks/CodeTaskFactory.cs @@ -738,7 +738,7 @@ private Assembly CompileInMemoryAssembly() // Horrible code dom / compilation declarations var codeBuilder = new StringBuilder(); - var writer = new StringWriter(codeBuilder, CultureInfo.CurrentCulture); + using var writer = new StringWriter(codeBuilder, CultureInfo.CurrentCulture); var codeGeneratorOptions = new CodeGeneratorOptions { BlankLinesBetweenMembers = true, diff --git a/src/Tasks/DownloadFile.cs b/src/Tasks/DownloadFile.cs index efe54f514ca..93231a5db74 100644 --- a/src/Tasks/DownloadFile.cs +++ b/src/Tasks/DownloadFile.cs @@ -146,7 +146,8 @@ private async Task ExecuteAsync() private async Task DownloadAsync(Uri uri, CancellationToken cancellationToken) { // The main reason to use HttpClient vs WebClient is because we can pass a message handler for unit tests to mock - using (var client = new HttpClient(HttpMessageHandler ?? new HttpClientHandler(), disposeHandler: true) { Timeout = TimeSpan.FromMilliseconds(Timeout) }) + using var httpHandler = new HttpClientHandler(); + using (var client = new HttpClient(HttpMessageHandler ?? httpHandler, disposeHandler: true) { Timeout = TimeSpan.FromMilliseconds(Timeout) }) { // Only get the response without downloading the file so we can determine if the file is already up-to-date using (HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false)) diff --git a/src/Tasks/GenerateResource.cs b/src/Tasks/GenerateResource.cs index 572a19ea3bb..eaf9a7bca99 100644 --- a/src/Tasks/GenerateResource.cs +++ b/src/Tasks/GenerateResource.cs @@ -975,7 +975,7 @@ private bool IsDangerous(String filename) dangerous = false; FileStream stream = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.Read); - XmlTextReader reader = new XmlTextReader(stream); + using XmlTextReader reader = new XmlTextReader(stream); reader.DtdProcessing = DtdProcessing.Ignore; reader.XmlResolver = null; try @@ -1622,14 +1622,13 @@ private bool NeedToRebuildSourceFile(string sourceFilePath, DateTime sourceTime, private void GetStronglyTypedResourceToProcess(ref List inputsToProcess, ref List outputsToProcess) { bool needToRebuildSTR = false; + CodeDomProvider provider = null; // The resource file isn't out of date. So check whether the STR class file is. try { if (StronglyTypedFileName == null) { - CodeDomProvider provider = null; - if (ProcessResourceFiles.TryCreateCodeDomProvider(Log, StronglyTypedLanguage, out provider)) { StronglyTypedFileName = ProcessResourceFiles.GenerateDefaultStronglyTypedFilename(provider, OutputResources[0].ItemSpec); @@ -1645,6 +1644,10 @@ private void GetStronglyTypedResourceToProcess(ref List inputsToProce _stronglyTypedResourceSuccessfullyCreated = false; return; } + finally + { + provider?.Dispose(); + } // Now we have the filename, check if it's up to date DateTime sourceTime = NativeMethodsShared.GetLastWriteFileUtcTime(Sources[0].ItemSpec); @@ -2153,11 +2156,20 @@ private void RecordFilesWritten() { if (StronglyTypedFileName == null) { - CodeDomProvider provider; - if (ProcessResourceFiles.TryCreateCodeDomProvider(Log, StronglyTypedLanguage, out provider)) + CodeDomProvider provider = null; + try + { + if (ProcessResourceFiles.TryCreateCodeDomProvider(Log, StronglyTypedLanguage, out provider)) + { + StronglyTypedFileName = ProcessResourceFiles.GenerateDefaultStronglyTypedFilename( + provider, OutputResources[0].ItemSpec); + + provider.Dispose(); + } + } + finally { - StronglyTypedFileName = ProcessResourceFiles.GenerateDefaultStronglyTypedFilename( - provider, OutputResources[0].ItemSpec); + provider?.Dispose(); } } @@ -3412,11 +3424,18 @@ private bool HaveSystemResourcesExtensionsReference /// The generated strongly typed filename private void CreateStronglyTypedResources(ReaderInfo reader, String outFile, String inputFileName, out String sourceFile) { - CodeDomProvider provider; - if (!TryCreateCodeDomProvider(_logger, _stronglyTypedLanguage, out provider)) + CodeDomProvider provider = null; + try { - sourceFile = null; - return; + if (!TryCreateCodeDomProvider(_logger, _stronglyTypedLanguage, out provider)) + { + sourceFile = null; + return; + } + } + finally + { + provider?.Dispose(); } // Default the class name if we need to @@ -3542,15 +3561,16 @@ private void ReadResources(ReaderInfo readerInfo, IResourceReader reader, String #endif // FEATURE_RESXREADER_LIVEDESERIALIZATION /// - /// Read resources from a text format file + /// Read resources from a text format file. /// - /// Reader info - /// Input resources filename + /// Reader info. + /// Input resources filename. private void ReadTextResources(ReaderInfo reader, String fileName) { // Check for byte order marks in the beginning of the input file, but // default to UTF-8. - using (LineNumberStreamReader sr = new LineNumberStreamReader(fileName, new UTF8Encoding(true), true)) + using var fs = File.Open(fileName, FileMode.Open, FileAccess.Read, FileShare.Read); + using (LineNumberStreamReader sr = new LineNumberStreamReader(fs, new UTF8Encoding(true), true)) { StringBuilder name = new StringBuilder(255); StringBuilder value = new StringBuilder(2048); @@ -3876,8 +3896,8 @@ internal sealed class LineNumberStreamReader : StreamReader private int _lineNumber; private int _col; - internal LineNumberStreamReader(String fileName, Encoding encoding, bool detectEncoding) - : base(File.Open(fileName, FileMode.Open, FileAccess.Read, FileShare.Read), encoding, detectEncoding) + internal LineNumberStreamReader(Stream fileStream, Encoding encoding, bool detectEncoding) + : base(fileStream, encoding, detectEncoding) { _lineNumber = 1; _col = 0; diff --git a/src/Tasks/GetAssembliesMetadata.cs b/src/Tasks/GetAssembliesMetadata.cs index d5f67c62f36..e49a0a2f713 100644 --- a/src/Tasks/GetAssembliesMetadata.cs +++ b/src/Tasks/GetAssembliesMetadata.cs @@ -50,7 +50,7 @@ public override bool Execute() // During DTB the referenced project may not has been built yet, so we need to check if the assembly already exists. if (File.Exists(assemblyPath)) { - AssemblyInformation assemblyInformation = new(assemblyPath); + using AssemblyInformation assemblyInformation = new(assemblyPath); AssemblyAttributes attributes = assemblyInformation.GetAssemblyMetadata(); if (attributes != null) diff --git a/src/Tasks/GetInstalledSDKLocations.cs b/src/Tasks/GetInstalledSDKLocations.cs index 011ce919725..8f4bb052bd9 100644 --- a/src/Tasks/GetInstalledSDKLocations.cs +++ b/src/Tasks/GetInstalledSDKLocations.cs @@ -194,7 +194,7 @@ public override bool Execute() object staticCacheDisposer = buildEngine4.GetRegisteredTaskObject(StaticSDKCacheKey, RegisteredTaskObjectLifetime.Build); if (staticCacheDisposer == null) { - BuildCacheDisposeWrapper staticDisposer = new BuildCacheDisposeWrapper(ToolLocationHelper.ClearSDKStaticCache); + using BuildCacheDisposeWrapper staticDisposer = new BuildCacheDisposeWrapper(ToolLocationHelper.ClearSDKStaticCache); buildEngine4.RegisterTaskObject(StaticSDKCacheKey, staticDisposer, RegisteredTaskObjectLifetime.Build, allowEarlyCollection: false); } } diff --git a/src/Tasks/ManifestUtil/ManifestFormatter.cs b/src/Tasks/ManifestUtil/ManifestFormatter.cs index a4295b3da34..7171fff8e71 100644 --- a/src/Tasks/ManifestUtil/ManifestFormatter.cs +++ b/src/Tasks/ManifestUtil/ManifestFormatter.cs @@ -17,7 +17,7 @@ public static Stream Format(Stream input) { int t1 = Environment.TickCount; - var r = new XmlTextReader(input) + using var r = new XmlTextReader(input) { DtdProcessing = DtdProcessing.Ignore, WhitespaceHandling = WhitespaceHandling.None @@ -25,7 +25,7 @@ public static Stream Format(Stream input) XmlNamespaceManager nsmgr = XmlNamespaces.GetNamespaceManager(r.NameTable); var m = new MemoryStream(); - var w = new XmlTextWriter(m, Encoding.UTF8) + using var w = new XmlTextWriter(m, Encoding.UTF8) { Formatting = Formatting.Indented, Indentation = 2 diff --git a/src/Tasks/ManifestUtil/ManifestReader.cs b/src/Tasks/ManifestUtil/ManifestReader.cs index fc4afee3919..f10c0d2a963 100644 --- a/src/Tasks/ManifestUtil/ManifestReader.cs +++ b/src/Tasks/ManifestUtil/ManifestReader.cs @@ -224,7 +224,7 @@ public static Manifest ReadManifest(string manifestType, Stream input, bool pres private static Manifest Deserialize(Stream s) { s.Position = 0; - var r = new XmlTextReader(s) { DtdProcessing = DtdProcessing.Ignore }; + using var r = new XmlTextReader(s) { DtdProcessing = DtdProcessing.Ignore }; do { diff --git a/src/Tasks/ManifestUtil/ManifestWriter.cs b/src/Tasks/ManifestUtil/ManifestWriter.cs index 562cc1f1c0f..45d6c4ddd18 100644 --- a/src/Tasks/ManifestUtil/ManifestWriter.cs +++ b/src/Tasks/ManifestUtil/ManifestWriter.cs @@ -24,7 +24,7 @@ private static Stream Serialize(Manifest manifest) manifest.OnBeforeSave(); var m = new MemoryStream(); var s = new XmlSerializer(manifest.GetType()); - var w = new StreamWriter(m); + using var w = new StreamWriter(m); int t1 = Environment.TickCount; s.Serialize(w, manifest); diff --git a/src/Tasks/ManifestUtil/SecurityUtil.cs b/src/Tasks/ManifestUtil/SecurityUtil.cs index 9951399b793..abe7dc3277e 100644 --- a/src/Tasks/ManifestUtil/SecurityUtil.cs +++ b/src/Tasks/ManifestUtil/SecurityUtil.cs @@ -205,7 +205,7 @@ private static XmlElement GetXmlElement(string targetZone, FrameworkName fn) { try { - var sr = new StreamReader(fs); + using var sr = new StreamReader(fs); string data = sr.ReadToEnd(); if (!string.IsNullOrEmpty(data)) { @@ -610,7 +610,7 @@ public static void SignFile(string certThumbprint, [SupportedOSPlatform("windows")] public static void SignFile(string certPath, SecureString certPassword, Uri timestampUrl, string path) { - X509Certificate2 cert = new X509Certificate2(certPath, certPassword, X509KeyStorageFlags.PersistKeySet); + using X509Certificate2 cert = new X509Certificate2(certPath, certPassword, X509KeyStorageFlags.PersistKeySet); SignFile(cert, timestampUrl, path); } @@ -705,7 +705,7 @@ private static void SignFileInternal(X509Certificate2 cert, CmiManifestSigner2 signer; if (useSha256 && rsa is RSACryptoServiceProvider rsacsp) { - RSACryptoServiceProvider csp = SignedCmiManifest2.GetFixedRSACryptoServiceProvider(rsacsp, useSha256); + using RSACryptoServiceProvider csp = SignedCmiManifest2.GetFixedRSACryptoServiceProvider(rsacsp, useSha256); signer = new CmiManifestSigner2(csp, cert, useSha256); } else diff --git a/src/Tasks/ManifestUtil/TrustInfo.cs b/src/Tasks/ManifestUtil/TrustInfo.cs index ecc02c975a7..8776175eddc 100644 --- a/src/Tasks/ManifestUtil/TrustInfo.cs +++ b/src/Tasks/ManifestUtil/TrustInfo.cs @@ -534,7 +534,8 @@ public override string ToString() var m = new MemoryStream(); Write(m); m.Position = 0; - var r = new StreamReader(m); + using var r = new StreamReader(m); + return r.ReadToEnd(); } diff --git a/src/Tasks/ManifestUtil/Util.cs b/src/Tasks/ManifestUtil/Util.cs index 4d6b6ca09ea..a83a5b7938b 100644 --- a/src/Tasks/ManifestUtil/Util.cs +++ b/src/Tasks/ManifestUtil/Util.cs @@ -209,7 +209,8 @@ public static Version GetTargetFrameworkVersion(string targetFramework) public static string GetEmbeddedResourceString(string name) { Stream s = GetEmbeddedResourceStream(name); - StreamReader r = new StreamReader(s); + using StreamReader r = new StreamReader(s); + return r.ReadToEnd(); } @@ -241,30 +242,37 @@ private static void GetFileInfoImpl(string path, string targetFrameWorkVersion, try { s = fi.OpenRead(); - HashAlgorithm hashAlg; + HashAlgorithm hashAlg = null; - if (string.IsNullOrEmpty(targetFrameWorkVersion) || CompareFrameworkVersions(targetFrameWorkVersion, Constants.TargetFrameworkVersion40) <= 0) + try { + if (string.IsNullOrEmpty(targetFrameWorkVersion) || CompareFrameworkVersions(targetFrameWorkVersion, Constants.TargetFrameworkVersion40) <= 0) + { #pragma warning disable SA1111, SA1009 // Closing parenthesis should be on line of last parameter - hashAlg = SHA1.Create( + hashAlg = SHA1.Create( #if FEATURE_CRYPTOGRAPHIC_FACTORY_ALGORITHM_NAMES - "System.Security.Cryptography.SHA1CryptoServiceProvider" + "System.Security.Cryptography.SHA1CryptoServiceProvider" #endif - ); + ); #pragma warning restore SA1111, SA1009 // Closing parenthesis should be on line of last parameter - } - else - { + } + else + { #pragma warning disable SA1111, SA1009 // Closing parenthesis should be on line of last parameter - hashAlg = SHA256.Create( + hashAlg = SHA256.Create( #if FEATURE_CRYPTOGRAPHIC_FACTORY_ALGORITHM_NAMES - "System.Security.Cryptography.SHA256CryptoServiceProvider" + "System.Security.Cryptography.SHA256CryptoServiceProvider" #endif - ); + ); #pragma warning restore SA1111, SA1009 // Closing parenthesis should be on line of last parameter + } + byte[] hashBytes = hashAlg.ComputeHash(s); + hash = Convert.ToBase64String(hashBytes); + } + finally + { + hashAlg?.Dispose(); } - byte[] hashBytes = hashAlg.ComputeHash(s); - hash = Convert.ToBase64String(hashBytes); } finally { @@ -473,7 +481,7 @@ public static void WriteFile(string path, string s) public static void WriteFile(string path, Stream s) { - StreamReader r = new StreamReader(s); + using StreamReader r = new StreamReader(s); WriteFile(path, r.ReadToEnd()); } @@ -520,7 +528,7 @@ public static void WriteLogFile(string filename, Stream s) } string path = Path.Combine(logPath, filename); - StreamReader r = new StreamReader(s); + using StreamReader r = new StreamReader(s); string text = r.ReadToEnd(); try { diff --git a/src/Tasks/ManifestUtil/XmlUtil.cs b/src/Tasks/ManifestUtil/XmlUtil.cs index ca35d8090a0..7ddb6a47536 100644 --- a/src/Tasks/ManifestUtil/XmlUtil.cs +++ b/src/Tasks/ManifestUtil/XmlUtil.cs @@ -115,7 +115,7 @@ public static Stream XslTransform(string resource, Stream input, params Dictiona } var m = new MemoryStream(); - var w = new XmlTextWriter(m, Encoding.UTF8); + using var w = new XmlTextWriter(m, Encoding.UTF8); w.WriteStartDocument(); int t5 = Environment.TickCount; diff --git a/src/Tasks/ManifestUtil/mansign2.cs b/src/Tasks/ManifestUtil/mansign2.cs index dfc7d8e46c2..1e98ca0ec72 100644 --- a/src/Tasks/ManifestUtil/mansign2.cs +++ b/src/Tasks/ManifestUtil/mansign2.cs @@ -511,7 +511,8 @@ private static void ReplacePublicKeyToken(XmlDocument manifestDom, AsymmetricAlg if (snKey is RSACryptoServiceProvider rsacsp) { - cspPublicKeyBlob = (GetFixedRSACryptoServiceProvider(rsacsp, useSha256)).ExportCspBlob(false); + using var cryptoProvider = GetFixedRSACryptoServiceProvider(rsacsp, useSha256); + cspPublicKeyBlob = cryptoProvider.ExportCspBlob(false); if (cspPublicKeyBlob == null || cspPublicKeyBlob.Length == 0) { throw new CryptographicException(Win32.NTE_BAD_KEY); diff --git a/src/Tasks/ResGen.cs b/src/Tasks/ResGen.cs index 5bce63d6e60..614ed571685 100644 --- a/src/Tasks/ResGen.cs +++ b/src/Tasks/ResGen.cs @@ -289,10 +289,11 @@ public override bool Execute() // Default the filename if we need to - regardless of whether the STR was successfully generated if (StronglyTypedFileName == null) { - CodeDomProvider provider; + CodeDomProvider provider = null; try { provider = CodeDomProvider.CreateProvider(StronglyTypedLanguage); + StronglyTypedFileName = ProcessResourceFiles.GenerateDefaultStronglyTypedFilename(provider, outputFile.ItemSpec); } catch (System.Configuration.ConfigurationException) { @@ -306,8 +307,10 @@ public override bool Execute() // logged an appropriate error. return false; } - - StronglyTypedFileName = ProcessResourceFiles.GenerateDefaultStronglyTypedFilename(provider, outputFile.ItemSpec); + finally + { + provider?.Dispose(); + } } } diff --git a/src/Tasks/ResolveKeySource.cs b/src/Tasks/ResolveKeySource.cs index deade89479b..2fcfbe94245 100644 --- a/src/Tasks/ResolveKeySource.cs +++ b/src/Tasks/ResolveKeySource.cs @@ -216,7 +216,7 @@ private bool ResolveManifestKey() { bool imported = false; // first try it with no password - var cert = new X509Certificate2(); + using var cert = new X509Certificate2(); var personalStore = new X509Store(StoreName.My, StoreLocation.CurrentUser); try { diff --git a/src/Tasks/ResourceHandling/FileStreamResource.cs b/src/Tasks/ResourceHandling/FileStreamResource.cs index ea29bf5b772..fa9d77dd964 100644 --- a/src/Tasks/ResourceHandling/FileStreamResource.cs +++ b/src/Tasks/ResourceHandling/FileStreamResource.cs @@ -12,8 +12,11 @@ namespace Microsoft.Build.Tasks.ResourceHandling internal class FileStreamResource : IResource { public string Name { get; } + public string TypeAssemblyQualifiedName { get; } + public string OriginatingFile { get; } + public string FileName { get; } public string TypeFullName => NameUtilities.FullNameFromAssemblyQualifiedName(TypeAssemblyQualifiedName); diff --git a/src/Tasks/Unzip.cs b/src/Tasks/Unzip.cs index 53ad3198125..20f9579a604 100644 --- a/src/Tasks/Unzip.cs +++ b/src/Tasks/Unzip.cs @@ -5,6 +5,7 @@ using System.IO; using System.IO.Compression; using System.Linq; +using System.Runtime.InteropServices.ComTypes; using System.Threading; using Microsoft.Build.Framework; @@ -98,6 +99,8 @@ public override bool Execute() BuildEngine3.Yield(); + FileStream stream = null; + ZipArchive zipArchive = null; try { ParseIncludeExclude(); @@ -114,9 +117,9 @@ public override bool Execute() try { - using (FileStream stream = new FileStream(sourceFile.ItemSpec, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 0x1000, useAsync: false)) + using (stream = new FileStream(sourceFile.ItemSpec, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 0x1000, useAsync: false)) { - using (ZipArchive zipArchive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: false)) + using (zipArchive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: false)) { try { @@ -146,6 +149,8 @@ public override bool Execute() finally { BuildEngine3.Reacquire(); + stream?.Dispose(); + zipArchive?.Dispose(); } return !_cancellationToken.IsCancellationRequested && !Log.HasLoggedErrors; diff --git a/src/Tasks/WriteCodeFragment.cs b/src/Tasks/WriteCodeFragment.cs index 77128537b7a..b586495371c 100644 --- a/src/Tasks/WriteCodeFragment.cs +++ b/src/Tasks/WriteCodeFragment.cs @@ -140,7 +140,7 @@ private string GenerateCode(out string extension) extension = null; bool haveGeneratedContent = false; - CodeDomProvider provider; + CodeDomProvider provider = null; try { @@ -156,6 +156,10 @@ private string GenerateCode(out string extension) Log.LogErrorWithCodeFromResources("WriteCodeFragment.CouldNotCreateProvider", Language, e.Message); return null; } + finally + { + provider?.Dispose(); + } extension = provider.FileExtension; diff --git a/src/Tasks/XamlTaskFactory/TaskParser.cs b/src/Tasks/XamlTaskFactory/TaskParser.cs index bd9f1aa2185..626a1a92587 100644 --- a/src/Tasks/XamlTaskFactory/TaskParser.cs +++ b/src/Tasks/XamlTaskFactory/TaskParser.cs @@ -144,7 +144,9 @@ private bool ParseAsContentOrFile(string contentOrFile, string desiredRule) throw new ArgumentException(ResourceUtilities.FormatResourceStringStripCodeAndKeyword("Xaml.RuleFileNotFound", contentOrFile)); } - return ParseXamlDocument(new StreamReader(contentOrFile), desiredRule); + using var sr = new StreamReader(contentOrFile); + + return ParseXamlDocument(sr, desiredRule); } // On Windows, xml content string is not a valid path, so, maybeFullPath == null @@ -158,7 +160,9 @@ private bool ParseAsContentOrFile(string contentOrFile, string desiredRule) if (FileSystems.Default.FileExists(maybeFullPath)) { // file found, parse as a file - return ParseXamlDocument(new StreamReader(maybeFullPath), desiredRule); + using var sr = new StreamReader(maybeFullPath); + + return ParseXamlDocument(sr, desiredRule); } // @maybeFullPath is either: diff --git a/src/Tasks/XamlTaskFactory/XamlTaskFactory.cs b/src/Tasks/XamlTaskFactory/XamlTaskFactory.cs index 4094d25b62e..342fb1f30b2 100644 --- a/src/Tasks/XamlTaskFactory/XamlTaskFactory.cs +++ b/src/Tasks/XamlTaskFactory/XamlTaskFactory.cs @@ -131,7 +131,7 @@ public bool Initialize(string taskName, IDictionary ta }; // create the code provider - var codegenerator = CodeDomProvider.CreateProvider("cs"); + using var codegenerator = CodeDomProvider.CreateProvider("cs"); CompilerResults results; bool debugXamlTask = Environment.GetEnvironmentVariable("MSBUILDWRITEXAMLTASK") == "1"; if (debugXamlTask) diff --git a/src/Tasks/XslTransformation.cs b/src/Tasks/XslTransformation.cs index 2f255f143cc..f55532b9546 100644 --- a/src/Tasks/XslTransformation.cs +++ b/src/Tasks/XslTransformation.cs @@ -451,8 +451,12 @@ public XslCompiledTransform LoadXslt(bool useTrustedSettings) switch (_xslMode) { case XslModes.Xslt: - xslct.Load(XmlReader.Create(new StringReader(_data)), settings, new XmlUrlResolver()); - break; + { + using var sr = new StringReader(_data); + using var xmlReader = XmlReader.Create(sr); + xslct.Load(xmlReader, settings, new XmlUrlResolver()); + break; + } case XslModes.XsltFile: if (useTrustedSettings) { diff --git a/src/UnitTests.Shared/ObjectModelHelpers.cs b/src/UnitTests.Shared/ObjectModelHelpers.cs index ce51be22785..e5cf81e5fe3 100644 --- a/src/UnitTests.Shared/ObjectModelHelpers.cs +++ b/src/UnitTests.Shared/ObjectModelHelpers.cs @@ -1971,7 +1971,8 @@ public static void VerifyAssertLineByLine(string expected, string actual, bool i /// public static void ClearDirtyFlag(ProjectRootElement project) { - project.Save(new StringWriter()); + using var sw = new StringWriter(); + project.Save(sw); Assert.False(project.HasUnsavedChanges); } From 7ed50851db68d4bb947b661f4e12eb03f87f78bc Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Thu, 11 Apr 2024 11:35:47 +0200 Subject: [PATCH 09/20] fix review comments --- .../BackEnd/BuildManager/BuildManager.cs | 116 +++++++++--------- .../BinaryLogger/BuildEventArgsReader.cs | 8 +- .../Postprocessing/StreamExtensions.cs | 1 - .../Engine/LocalProvider/LocalNode.cs | 1 + src/Framework/NativeMethods.cs | 4 +- src/Shared/NodeEndpointOutOfProcBase.cs | 3 + src/Tasks/DownloadFile.cs | 5 +- src/Tasks/ManifestUtil/XmlUtil.cs | 2 +- .../ResourceHandling/FileStreamResource.cs | 2 + 9 files changed, 73 insertions(+), 69 deletions(-) diff --git a/src/Build/BackEnd/BuildManager/BuildManager.cs b/src/Build/BackEnd/BuildManager/BuildManager.cs index 98c7f61606c..3c31dce9106 100644 --- a/src/Build/BackEnd/BuildManager/BuildManager.cs +++ b/src/Build/BackEnd/BuildManager/BuildManager.cs @@ -2005,83 +2005,81 @@ private Dictionary BuildGraph( GraphBuildRequestData graphBuildRequestData) { var resultsPerNode = new Dictionary(projectGraph.ProjectNodes.Count); - using (var waitHandle = new AutoResetEvent(true)) - { - var graphBuildStateLock = new object(); + using var waitHandle = new AutoResetEvent(true); + var graphBuildStateLock = new object(); - var blockedNodes = new HashSet(projectGraph.ProjectNodes); - var finishedNodes = new HashSet(projectGraph.ProjectNodes.Count); - var buildingNodes = new Dictionary(); - ExceptionDispatchInfo submissionException = null; + var blockedNodes = new HashSet(projectGraph.ProjectNodes); + var finishedNodes = new HashSet(projectGraph.ProjectNodes.Count); + var buildingNodes = new Dictionary(); + ExceptionDispatchInfo submissionException = null; - while (blockedNodes.Count > 0 || buildingNodes.Count > 0) - { - waitHandle.WaitOne(); + while (blockedNodes.Count > 0 || buildingNodes.Count > 0) + { + waitHandle.WaitOne(); - // When a cache plugin is present, ExecuteSubmission(BuildSubmission) executes on a separate thread whose exceptions do not get observed. - // Observe them here to keep the same exception flow with the case when there's no plugins and ExecuteSubmission(BuildSubmission) does not run on a separate thread. - if (submissionException != null) - { - submissionException.Throw(); - } + // When a cache plugin is present, ExecuteSubmission(BuildSubmission) executes on a separate thread whose exceptions do not get observed. + // Observe them here to keep the same exception flow with the case when there's no plugins and ExecuteSubmission(BuildSubmission) does not run on a separate thread. + if (submissionException != null) + { + submissionException.Throw(); + } - lock (graphBuildStateLock) + lock (graphBuildStateLock) + { + var unblockedNodes = blockedNodes + .Where(node => node.ProjectReferences.All(projectReference => finishedNodes.Contains(projectReference))) + .ToList(); + foreach (var node in unblockedNodes) { - var unblockedNodes = blockedNodes - .Where(node => node.ProjectReferences.All(projectReference => finishedNodes.Contains(projectReference))) - .ToList(); - foreach (var node in unblockedNodes) + var targetList = targetsPerNode[node]; + if (targetList.Count == 0) { - var targetList = targetsPerNode[node]; - if (targetList.Count == 0) - { - // An empty target list here means "no targets" instead of "default targets", so don't even build it. - finishedNodes.Add(node); - blockedNodes.Remove(node); + // An empty target list here means "no targets" instead of "default targets", so don't even build it. + finishedNodes.Add(node); + blockedNodes.Remove(node); - waitHandle.Set(); + waitHandle.Set(); - continue; - } + continue; + } - var request = new BuildRequestData( - node.ProjectInstance, - targetList.ToArray(), - graphBuildRequestData.HostServices, - graphBuildRequestData.Flags); - - // TODO Tack onto the existing submission instead of pending a whole new submission for every node - // Among other things, this makes BuildParameters.DetailedSummary produce a summary for each node, which is not desirable. - // We basically want to submit all requests to the scheduler all at once and describe dependencies by requests being blocked by other requests. - // However today the scheduler only keeps track of MSBuild nodes being blocked by other MSBuild nodes, and MSBuild nodes haven't been assigned to the graph nodes yet. - var innerBuildSubmission = PendBuildRequest(request); - buildingNodes.Add(innerBuildSubmission, node); - blockedNodes.Remove(node); - innerBuildSubmission.ExecuteAsync(finishedBuildSubmission => + var request = new BuildRequestData( + node.ProjectInstance, + targetList.ToArray(), + graphBuildRequestData.HostServices, + graphBuildRequestData.Flags); + + // TODO Tack onto the existing submission instead of pending a whole new submission for every node + // Among other things, this makes BuildParameters.DetailedSummary produce a summary for each node, which is not desirable. + // We basically want to submit all requests to the scheduler all at once and describe dependencies by requests being blocked by other requests. + // However today the scheduler only keeps track of MSBuild nodes being blocked by other MSBuild nodes, and MSBuild nodes haven't been assigned to the graph nodes yet. + var innerBuildSubmission = PendBuildRequest(request); + buildingNodes.Add(innerBuildSubmission, node); + blockedNodes.Remove(node); + innerBuildSubmission.ExecuteAsync(finishedBuildSubmission => + { + lock (graphBuildStateLock) { - lock (graphBuildStateLock) + if (submissionException == null && finishedBuildSubmission.BuildResult.Exception != null) { - if (submissionException == null && finishedBuildSubmission.BuildResult.Exception != null) - { - // Preserve the original stack. - submissionException = ExceptionDispatchInfo.Capture(finishedBuildSubmission.BuildResult.Exception); - } + // Preserve the original stack. + submissionException = ExceptionDispatchInfo.Capture(finishedBuildSubmission.BuildResult.Exception); + } - ProjectGraphNode finishedNode = buildingNodes[finishedBuildSubmission]; + ProjectGraphNode finishedNode = buildingNodes[finishedBuildSubmission]; - finishedNodes.Add(finishedNode); - buildingNodes.Remove(finishedBuildSubmission); + finishedNodes.Add(finishedNode); + buildingNodes.Remove(finishedBuildSubmission); - resultsPerNode.Add(finishedNode, finishedBuildSubmission.BuildResult); - } + resultsPerNode.Add(finishedNode, finishedBuildSubmission.BuildResult); + } - waitHandle.Set(); - }, null); - } + waitHandle.Set(); + }, null); } } } - + return resultsPerNode; } diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs index 720ef5b0fd1..72a3629a0e2 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs @@ -20,7 +20,7 @@ namespace Microsoft.Build.Logging { /// - /// Deserializes and returns BuildEventArgs-derived objects from a BinaryReader + /// Deserializes and returns BuildEventArgs-derived objects from a BinaryReader. /// public class BuildEventArgsReader : IBuildEventArgsReaderNotifications, IDisposable { @@ -185,10 +185,8 @@ internal RawRecord ReadRaw() int serializedEventLength = ReadInt32(); Stream stream = _binaryReader.BaseStream.Slice(serializedEventLength); - using (_lastSubStream = stream as SubStream) - { - _recordNumber += 1; - } + _lastSubStream = stream as SubStream; + _recordNumber += 1; return new(recordKind, stream); } diff --git a/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs b/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs index 7f2114819a3..68c15b3288a 100644 --- a/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs +++ b/src/Build/Logging/BinaryLogger/Postprocessing/StreamExtensions.cs @@ -60,7 +60,6 @@ public static byte[] ReadToEnd(this Stream stream) { if (stream.TryGetLength(out long length)) { - // check with Jan using BinaryReader reader = new(stream); return reader.ReadBytes((int)length); diff --git a/src/Deprecated/Engine/LocalProvider/LocalNode.cs b/src/Deprecated/Engine/LocalProvider/LocalNode.cs index 73869190849..19948970f29 100644 --- a/src/Deprecated/Engine/LocalProvider/LocalNode.cs +++ b/src/Deprecated/Engine/LocalProvider/LocalNode.cs @@ -217,6 +217,7 @@ private static bool CreateGlobalEvents(int nodeNumber) /// This function starts local node when process is launched and shuts it down on time out /// Called by msbuild.exe. /// + [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Agreed not to touch entries from Deprecated folder")] public static void StartLocalNodeServer(int nodeNumber) { // Create global events necessary for handshaking with the parent diff --git a/src/Framework/NativeMethods.cs b/src/Framework/NativeMethods.cs index 093ad6113bd..9cff18f438d 100644 --- a/src/Framework/NativeMethods.cs +++ b/src/Framework/NativeMethods.cs @@ -1340,7 +1340,9 @@ internal static List> GetChildProcessIds(in { // Hold the child process handle open so that children cannot die and restart with a different parent after we've started looking at it. // This way, any handle we pass back is guaranteed to be one of our actual children. - using (SafeProcessHandle childHandle = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, possibleChildProcess.Id)) +#pragma warning disable CA2000 // Dispose objects before losing scope by design + SafeProcessHandle childHandle = OpenProcess(eDesiredAccess.PROCESS_QUERY_INFORMATION, false, possibleChildProcess.Id); +#pragma warning restore CA2000 // Dispose objects before losing scope { if (childHandle.IsInvalid) { diff --git a/src/Shared/NodeEndpointOutOfProcBase.cs b/src/Shared/NodeEndpointOutOfProcBase.cs index 6c12448b1ad..8783318b2e5 100644 --- a/src/Shared/NodeEndpointOutOfProcBase.cs +++ b/src/Shared/NodeEndpointOutOfProcBase.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Diagnostics.CodeAnalysis; #if CLR2COMPATIBILITY using Microsoft.Build.Shared.Concurrent; #else @@ -14,6 +15,7 @@ using Microsoft.Build.Shared; #if FEATURE_SECURITY_PERMISSIONS || FEATURE_PIPE_SECURITY using System.Security.AccessControl; + #endif #if FEATURE_PIPE_SECURITY && FEATURE_NAMED_PIPE_SECURITY_CONSTRUCTOR using System.Security.Principal; @@ -29,6 +31,7 @@ namespace Microsoft.Build.BackEnd /// /// This is an implementation of INodeEndpoint for the out-of-proc nodes. It acts only as a client. /// + [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "It is expected to keep the stream open for the process lifetime")] internal abstract class NodeEndpointOutOfProcBase : INodeEndpoint { #region Private Data diff --git a/src/Tasks/DownloadFile.cs b/src/Tasks/DownloadFile.cs index 93231a5db74..f8a57d147a6 100644 --- a/src/Tasks/DownloadFile.cs +++ b/src/Tasks/DownloadFile.cs @@ -146,8 +146,8 @@ private async Task ExecuteAsync() private async Task DownloadAsync(Uri uri, CancellationToken cancellationToken) { // The main reason to use HttpClient vs WebClient is because we can pass a message handler for unit tests to mock - using var httpHandler = new HttpClientHandler(); - using (var client = new HttpClient(HttpMessageHandler ?? httpHandler, disposeHandler: true) { Timeout = TimeSpan.FromMilliseconds(Timeout) }) +#pragma warning disable CA2000 // Dispose objects before losing scope because the HttpClient is disposed by HTTPClient.Dispose() + using (var client = new HttpClient(HttpMessageHandler ?? new HttpClientHandler(), disposeHandler: true) { Timeout = TimeSpan.FromMilliseconds(Timeout) }) { // Only get the response without downloading the file so we can determine if the file is already up-to-date using (HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false)) @@ -227,6 +227,7 @@ private async Task DownloadAsync(Uri uri, CancellationToken cancellationToken) } } } +#pragma warning restore CA2000 // Dispose objects before losing scope } /// diff --git a/src/Tasks/ManifestUtil/XmlUtil.cs b/src/Tasks/ManifestUtil/XmlUtil.cs index 7ddb6a47536..709aaa1e9e3 100644 --- a/src/Tasks/ManifestUtil/XmlUtil.cs +++ b/src/Tasks/ManifestUtil/XmlUtil.cs @@ -114,7 +114,7 @@ public static Stream XslTransform(string resource, Stream input, params Dictiona } } - var m = new MemoryStream(); + using var m = new MemoryStream(); using var w = new XmlTextWriter(m, Encoding.UTF8); w.WriteStartDocument(); diff --git a/src/Tasks/ResourceHandling/FileStreamResource.cs b/src/Tasks/ResourceHandling/FileStreamResource.cs index fa9d77dd964..39117f25e70 100644 --- a/src/Tasks/ResourceHandling/FileStreamResource.cs +++ b/src/Tasks/ResourceHandling/FileStreamResource.cs @@ -40,7 +40,9 @@ public void AddTo(IResourceWriter writer) { if (writer is PreserializedResourceWriter preserializedResourceWriter) { +#pragma warning disable CA2000 // Dispose objects before losing scope the stream is expected to be disposed by the PreserializedResourceWriter.ResourceDataRecord FileStream fileStream = new FileStream(FileName, FileMode.Open, FileAccess.Read, FileShare.Read); +#pragma warning restore CA2000 // Dispose objects before losing scope preserializedResourceWriter.AddActivatorResource(Name, fileStream, TypeAssemblyQualifiedName, closeAfterWrite: true); } From ac0bd8881286a6274b97c74fb8b5d147946d6df4 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova Date: Thu, 11 Apr 2024 12:45:19 +0200 Subject: [PATCH 10/20] enable rule globaly, but exclude tests --- .editorconfig | 5 ++++- .../Microsoft.Build.Engine.OM.UnitTests.csproj | 1 + src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj | 1 + .../Microsoft.Build.Framework.UnitTests.csproj | 1 + .../Microsoft.Build.CommandLine.UnitTests.csproj | 1 + src/StringTools.UnitTests/StringTools.UnitTests.csproj | 1 + src/StringTools.UnitTests/StringTools.UnitTests.net35.csproj | 1 + src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj | 1 + src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj | 1 + .../Microsoft.Build.Utilities.UnitTests.csproj | 1 + 10 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.editorconfig b/.editorconfig index 7b0f1419bb8..93a196caa51 100644 --- a/.editorconfig +++ b/.editorconfig @@ -1,4 +1,4 @@ -# editorconfig.org +# editorconfig.org # top-most EditorConfig file root = true @@ -163,6 +163,9 @@ dotnet_code_quality.ca2208.api_surface = public # CA1852: Seal internal types dotnet_diagnostic.ca1852.severity = warning +# CA2000: Dispose objects before losing scope +dotnet_diagnostic.ca2000.severity = error + # RS0037: Enable tracking of nullability of reference types in the declared API # Our API is not annotated but new classes get nullable enabled so disable this. # We'd be happy if everything was annotated and this could be removed. diff --git a/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj b/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj index df1c15ea5e6..66b799a019f 100644 --- a/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj +++ b/src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj @@ -12,6 +12,7 @@ true $(DefineConstants);MICROSOFT_BUILD_ENGINE_OM_UNITTESTS;NO_FRAMEWORK_IVT + $(NoWarn);CA2000 diff --git a/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj b/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj index b9c1cefc88c..4829996a0f4 100644 --- a/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj +++ b/src/Build.UnitTests/Microsoft.Build.Engine.UnitTests.csproj @@ -12,6 +12,7 @@ $(DefineConstants);NO_MSBUILDTASKHOST true + $(NoWarn);CA2000 diff --git a/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj b/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj index f5931d3d9af..f668bd93110 100644 --- a/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj +++ b/src/Framework.UnitTests/Microsoft.Build.Framework.UnitTests.csproj @@ -5,6 +5,7 @@ $(RuntimeOutputPlatformTarget) True Microsoft.Build.Framework.UnitTests + $(NoWarn);CA2000 diff --git a/src/MSBuild.UnitTests/Microsoft.Build.CommandLine.UnitTests.csproj b/src/MSBuild.UnitTests/Microsoft.Build.CommandLine.UnitTests.csproj index 35fa1dbb627..787d5ea4979 100644 --- a/src/MSBuild.UnitTests/Microsoft.Build.CommandLine.UnitTests.csproj +++ b/src/MSBuild.UnitTests/Microsoft.Build.CommandLine.UnitTests.csproj @@ -4,6 +4,7 @@ $(RuntimeOutputTargetFrameworks) $(RuntimeOutputPlatformTarget) false + $(NoWarn);CA2000 diff --git a/src/StringTools.UnitTests/StringTools.UnitTests.csproj b/src/StringTools.UnitTests/StringTools.UnitTests.csproj index b48cd46cb93..c9ab3135d37 100644 --- a/src/StringTools.UnitTests/StringTools.UnitTests.csproj +++ b/src/StringTools.UnitTests/StringTools.UnitTests.csproj @@ -8,6 +8,7 @@ Microsoft.NET.StringTools.UnitTests true true + $(NoWarn);CA2000 diff --git a/src/StringTools.UnitTests/StringTools.UnitTests.net35.csproj b/src/StringTools.UnitTests/StringTools.UnitTests.net35.csproj index acea1b5a025..31a5634c2f4 100644 --- a/src/StringTools.UnitTests/StringTools.UnitTests.net35.csproj +++ b/src/StringTools.UnitTests/StringTools.UnitTests.net35.csproj @@ -14,6 +14,7 @@ true true $(DefineConstants);NET35_UNITTEST + $(NoWarn);CA2000 diff --git a/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj b/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj index 2c1fca47574..ec5853b9868 100644 --- a/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj +++ b/src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj @@ -9,6 +9,7 @@ Microsoft.Build.Tasks.UnitTests true $(DefineConstants);MICROSOFT_BUILD_TASKS_UNITTESTS + $(NoWarn);CA2000 diff --git a/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj b/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj index 0bade6a09d5..de40cc157ca 100644 --- a/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj +++ b/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj @@ -6,6 +6,7 @@ false false true + $(NoWarn);CA2000 @@ -14,7 +14,7 @@ true true $(DefineConstants);NET35_UNITTEST - $(NoWarn);CA2000 + $(NoWarn);CA2000 diff --git a/src/Tasks/AppConfig/AppConfig.cs b/src/Tasks/AppConfig/AppConfig.cs index 40932d287a5..0e746a573e7 100644 --- a/src/Tasks/AppConfig/AppConfig.cs +++ b/src/Tasks/AppConfig/AppConfig.cs @@ -34,10 +34,10 @@ internal void Load(string appConfigFilePath) // Need a filestream as the XmlReader doesn't support nonstandard unicode characters in path. // No need to dispose - as 'CloseInput' was passed to XmlReaderSettings FileStream fs = File.OpenRead(appConfigFilePath); - using (reader = XmlReader.Create(fs, readerSettings)) - { - Read(reader); - } +#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because the reader is disposed in the finally block + reader = XmlReader.Create(fs, readerSettings); +#pragma warning restore CA2000 // Dispose objects before losing scope + Read(reader); } catch (XmlException e) { diff --git a/src/Tasks/GenerateResource.cs b/src/Tasks/GenerateResource.cs index eaf9a7bca99..af988ec51d3 100644 --- a/src/Tasks/GenerateResource.cs +++ b/src/Tasks/GenerateResource.cs @@ -974,7 +974,7 @@ private bool IsDangerous(String filename) // XML files are only dangerous if there are unrecognized objects in them dangerous = false; - FileStream stream = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.Read); + using FileStream stream = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.Read); using XmlTextReader reader = new XmlTextReader(stream); reader.DtdProcessing = DtdProcessing.Ignore; reader.XmlResolver = null; @@ -2163,8 +2163,6 @@ private void RecordFilesWritten() { StronglyTypedFileName = ProcessResourceFiles.GenerateDefaultStronglyTypedFilename( provider, OutputResources[0].ItemSpec); - - provider.Dispose(); } } finally @@ -3432,60 +3430,61 @@ private void CreateStronglyTypedResources(ReaderInfo reader, String outFile, Str sourceFile = null; return; } - } - finally - { - provider?.Dispose(); - } - // Default the class name if we need to - if (_stronglyTypedClassName == null) - { - _stronglyTypedClassName = Path.GetFileNameWithoutExtension(outFile); - } - // Default the filename if we need to - if (_stronglyTypedFilename == null) - { - _stronglyTypedFilename = GenerateDefaultStronglyTypedFilename(provider, outFile); - } - sourceFile = this.StronglyTypedFilename; - - _logger.LogMessageFromResources("GenerateResource.CreatingSTR", _stronglyTypedFilename); - - // Generate the STR class - String[] errors; - bool generateInternalClass = !_stronglyTypedClassIsPublic; - // StronglyTypedResourcesNamespace can be null and this is ok. - // If it is null then the default namespace (=stronglyTypedNamespace) is used. - CodeCompileUnit ccu = StronglyTypedResourceBuilder.Create( - reader.resourcesHashTable, - _stronglyTypedClassName, - _stronglyTypedNamespace, - _stronglyTypedResourcesNamespace, - provider, - generateInternalClass, - out errors); + // Default the class name if we need to + if (_stronglyTypedClassName == null) + { + _stronglyTypedClassName = Path.GetFileNameWithoutExtension(outFile); + } - CodeGeneratorOptions codeGenOptions = new CodeGeneratorOptions(); - using (TextWriter output = new StreamWriter(_stronglyTypedFilename)) - { - provider.GenerateCodeFromCompileUnit(ccu, output, codeGenOptions); - } + // Default the filename if we need to + if (_stronglyTypedFilename == null) + { + _stronglyTypedFilename = GenerateDefaultStronglyTypedFilename(provider, outFile); + } + sourceFile = this.StronglyTypedFilename; + + _logger.LogMessageFromResources("GenerateResource.CreatingSTR", _stronglyTypedFilename); + + // Generate the STR class + String[] errors; + bool generateInternalClass = !_stronglyTypedClassIsPublic; + // StronglyTypedResourcesNamespace can be null and this is ok. + // If it is null then the default namespace (=stronglyTypedNamespace) is used. + CodeCompileUnit ccu = StronglyTypedResourceBuilder.Create( + reader.resourcesHashTable, + _stronglyTypedClassName, + _stronglyTypedNamespace, + _stronglyTypedResourcesNamespace, + provider, + generateInternalClass, + out errors); + + CodeGeneratorOptions codeGenOptions = new CodeGeneratorOptions(); + using (TextWriter output = new StreamWriter(_stronglyTypedFilename)) + { + provider.GenerateCodeFromCompileUnit(ccu, output, codeGenOptions); + } - if (errors.Length > 0) - { - _logger.LogErrorWithCodeFromResources("GenerateResource.ErrorFromCodeDom", inputFileName); - foreach (String error in errors) + if (errors.Length > 0) { - _logger.LogErrorWithCodeFromResources("GenerateResource.CodeDomError", error); + _logger.LogErrorWithCodeFromResources("GenerateResource.ErrorFromCodeDom", inputFileName); + foreach (String error in errors) + { + _logger.LogErrorWithCodeFromResources("GenerateResource.CodeDomError", error); + } + } + else + { + // No errors, and no exceptions - we presumably did create the STR class file + // and it should get added to FilesWritten. So set a flag to indicate this. + _stronglyTypedResourceSuccessfullyCreated = true; } } - else + finally { - // No errors, and no exceptions - we presumably did create the STR class file - // and it should get added to FilesWritten. So set a flag to indicate this. - _stronglyTypedResourceSuccessfullyCreated = true; + provider?.Dispose(); } } diff --git a/src/Tasks/GetInstalledSDKLocations.cs b/src/Tasks/GetInstalledSDKLocations.cs index 8f4bb052bd9..e1d4bb966e7 100644 --- a/src/Tasks/GetInstalledSDKLocations.cs +++ b/src/Tasks/GetInstalledSDKLocations.cs @@ -194,7 +194,9 @@ public override bool Execute() object staticCacheDisposer = buildEngine4.GetRegisteredTaskObject(StaticSDKCacheKey, RegisteredTaskObjectLifetime.Build); if (staticCacheDisposer == null) { - using BuildCacheDisposeWrapper staticDisposer = new BuildCacheDisposeWrapper(ToolLocationHelper.ClearSDKStaticCache); +#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because the object is registered with the engine and disposed of at the end of the build. + BuildCacheDisposeWrapper staticDisposer = new BuildCacheDisposeWrapper(ToolLocationHelper.ClearSDKStaticCache); +#pragma warning restore CA2000 // Dispose objects before losing scope buildEngine4.RegisterTaskObject(StaticSDKCacheKey, staticDisposer, RegisteredTaskObjectLifetime.Build, allowEarlyCollection: false); } } diff --git a/src/Tasks/ManifestUtil/ManifestWriter.cs b/src/Tasks/ManifestUtil/ManifestWriter.cs index 45d6c4ddd18..465fe49bbf6 100644 --- a/src/Tasks/ManifestUtil/ManifestWriter.cs +++ b/src/Tasks/ManifestUtil/ManifestWriter.cs @@ -24,7 +24,9 @@ private static Stream Serialize(Manifest manifest) manifest.OnBeforeSave(); var m = new MemoryStream(); var s = new XmlSerializer(manifest.GetType()); - using var w = new StreamWriter(m); +#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because the stream is returned to the caller and will be handled there. + var w = new StreamWriter(m); +#pragma warning restore CA2000 // Dispose objects before losing scope int t1 = Environment.TickCount; s.Serialize(w, manifest); @@ -32,6 +34,7 @@ private static Stream Serialize(Manifest manifest) w.Flush(); m.Position = 0; + return m; } diff --git a/src/Tasks/ManifestUtil/SecurityUtil.cs b/src/Tasks/ManifestUtil/SecurityUtil.cs index 6d5f37aba14..457265e0333 100644 --- a/src/Tasks/ManifestUtil/SecurityUtil.cs +++ b/src/Tasks/ManifestUtil/SecurityUtil.cs @@ -205,7 +205,9 @@ private static XmlElement GetXmlElement(string targetZone, FrameworkName fn) { try { - using var sr = new StreamReader(fs); +#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because it is managed by FileStream above. + var sr = new StreamReader(fs); +#pragma warning restore CA2000 // Dispose objects before losing scope string data = sr.ReadToEnd(); if (!string.IsNullOrEmpty(data)) { diff --git a/src/Tasks/ManifestUtil/Util.cs b/src/Tasks/ManifestUtil/Util.cs index e0852911d90..40f8c1bc221 100644 --- a/src/Tasks/ManifestUtil/Util.cs +++ b/src/Tasks/ManifestUtil/Util.cs @@ -521,7 +521,9 @@ public static void WriteLogFile(string filename, Stream s) } string path = Path.Combine(logPath, filename); - using StreamReader r = new StreamReader(s); +#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because the stream is returned to the caller and will be handled there. + StreamReader r = new StreamReader(s); +#pragma warning restore CA2000 // Dispose objects before losing scope string text = r.ReadToEnd(); try { @@ -539,6 +541,7 @@ public static void WriteLogFile(string filename, Stream s) catch (SecurityException) { } + s.Position = 0; } diff --git a/src/Tasks/Unzip.cs b/src/Tasks/Unzip.cs index 20f9579a604..6590a161c43 100644 --- a/src/Tasks/Unzip.cs +++ b/src/Tasks/Unzip.cs @@ -5,7 +5,6 @@ using System.IO; using System.IO.Compression; using System.Linq; -using System.Runtime.InteropServices.ComTypes; using System.Threading; using Microsoft.Build.Framework; @@ -99,8 +98,6 @@ public override bool Execute() BuildEngine3.Yield(); - FileStream stream = null; - ZipArchive zipArchive = null; try { ParseIncludeExclude(); @@ -117,9 +114,10 @@ public override bool Execute() try { - using (stream = new FileStream(sourceFile.ItemSpec, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 0x1000, useAsync: false)) + using (FileStream stream = new FileStream(sourceFile.ItemSpec, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 0x1000, useAsync: false)) { - using (zipArchive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: false)) +#pragma warning disable CA2000 // Dispose objects before losing scope because ZipArchive will dispose the stream when it is disposed. + using (ZipArchive zipArchive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: false)) { try { @@ -132,6 +130,7 @@ public override bool Execute() return false; } } +#pragma warning restore CA2000 // Dispose objects before losing scope } } catch (OperationCanceledException) @@ -149,8 +148,6 @@ public override bool Execute() finally { BuildEngine3.Reacquire(); - stream?.Dispose(); - zipArchive?.Dispose(); } return !_cancellationToken.IsCancellationRequested && !Log.HasLoggedErrors; diff --git a/src/Tasks/WriteCodeFragment.cs b/src/Tasks/WriteCodeFragment.cs index d59a89f4510..2576fa2b2b7 100644 --- a/src/Tasks/WriteCodeFragment.cs +++ b/src/Tasks/WriteCodeFragment.cs @@ -140,12 +140,24 @@ private string GenerateCode(out string extension) extension = null; bool haveGeneratedContent = false; - string code = string.Empty; CodeDomProvider provider = null; - try { - provider = CodeDomProvider.CreateProvider(Language); + try + { + provider = CodeDomProvider.CreateProvider(Language); + } + catch (SystemException e) when +#if FEATURE_SYSTEM_CONFIGURATION + (e is ConfigurationException || e is SecurityException) +#else + (e.GetType().Name == "ConfigurationErrorsException") // TODO: catch specific exception type once it is public https://github.com/dotnet/corefx/issues/40456 +#endif + { + Log.LogErrorWithCodeFromResources("WriteCodeFragment.CouldNotCreateProvider", Language, e.Message); + return null; + } + extension = provider.FileExtension; var unit = new CodeCompileUnit(); @@ -263,26 +275,19 @@ private string GenerateCode(out string extension) provider.GenerateCodeFromCompileUnit(unit, writer, new CodeGeneratorOptions()); } - code = generatedCode.ToString(); - } - catch (SystemException e) when -#if FEATURE_SYSTEM_CONFIGURATION - (e is ConfigurationException || e is SecurityException) -#else - (e.GetType().Name == "ConfigurationErrorsException") // TODO: catch specific exception type once it is public https://github.com/dotnet/corefx/issues/40456 -#endif - { - Log.LogErrorWithCodeFromResources("WriteCodeFragment.CouldNotCreateProvider", Language, e.Message); - return null; + string code = generatedCode.ToString(); + + // If we just generated infrastructure, don't bother returning anything + // as there's no point writing the file + return haveGeneratedContent ? code : String.Empty; } finally { - provider?.Dispose(); + if (provider != null) + { + provider.Dispose(); + } } - - // If we just generated infrastructure, don't bother returning anything - // as there's no point writing the file - return haveGeneratedContent ? code : String.Empty; } /// diff --git a/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj b/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj index 2533a8bd9d0..304b318e2b8 100644 --- a/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj +++ b/src/UnitTests.Shared/Microsoft.Build.UnitTests.Shared.csproj @@ -6,7 +6,7 @@ false false true - $(NoWarn);CA2000 + $(NoWarn);CA2000