diff --git a/eng/Versions.props b/eng/Versions.props index 3fcaf8be383..179e209e3be 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -14,6 +14,7 @@ true + true true true true diff --git a/src/Build.OM.UnitTests/Construction/ProjectRootElement_Tests.cs b/src/Build.OM.UnitTests/Construction/ProjectRootElement_Tests.cs index 1ff0a918ab2..8d354afd72e 100644 --- a/src/Build.OM.UnitTests/Construction/ProjectRootElement_Tests.cs +++ b/src/Build.OM.UnitTests/Construction/ProjectRootElement_Tests.cs @@ -9,6 +9,7 @@ using System.Security.AccessControl; using System.Security.Principal; #endif +using System.Reflection; using System.Text; using System.Threading; using System.Xml; @@ -18,6 +19,7 @@ using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException; using ProjectCollection = Microsoft.Build.Evaluation.ProjectCollection; +using Shouldly; using Xunit; namespace Microsoft.Build.UnitTests.OM.Construction @@ -1854,6 +1856,31 @@ public void ReloadCanOverwriteUnsavedChanges() AssertReload(SimpleProject, ComplexProject, true, true, true, act); } + [Fact] + public void ReloadDoesNotLeakCachedXmlDocuments() + { + using var env = TestEnvironment.Create(); + var testFiles = env.CreateTestProjectWithFiles("", new[] { "build.proj" }); + var projectFile = testFiles.CreatedFiles.First(); + + var projectElement = ObjectModelHelpers.CreateInMemoryProjectRootElement(SimpleProject); + projectElement.Save(projectFile); + + int originalDocumentCount = GetNumberOfDocumentsInProjectStringCache(projectElement); + + // Test successful reload. + projectElement.Reload(false); + GetNumberOfDocumentsInProjectStringCache(projectElement).ShouldBe(originalDocumentCount); + + // Test failed reload. + using (StreamWriter sw = new StreamWriter(projectFile)) + { + sw.WriteLine(""); // Invalid root element + } + Should.Throw(() => projectElement.Reload(false)); + GetNumberOfDocumentsInProjectStringCache(projectElement).ShouldBe(originalDocumentCount); + } + private void AssertReload( string initialContents, string changedContents, @@ -1986,5 +2013,17 @@ private void VerifyAssertLineByLine(string expected, string actual) { Helpers.VerifyAssertLineByLine(expected, actual, false); } + + /// + /// Returns the number of documents retained by the project string cache. + /// Peeks at it via reflection since internals are not visible to these tests. + /// + private int GetNumberOfDocumentsInProjectStringCache(ProjectRootElement project) + { + var bindingFlags = BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.GetProperty; + object document = typeof(ProjectRootElement).InvokeMember("XmlDocument", bindingFlags, null, project, Array.Empty()); + object cache = document.GetType().InvokeMember("StringCache", bindingFlags, null, document, Array.Empty()); + return (int)cache.GetType().InvokeMember("DocumentCount", bindingFlags, null, cache, Array.Empty()); + } } } diff --git a/src/Build/BackEnd/BuildManager/BuildManager.cs b/src/Build/BackEnd/BuildManager/BuildManager.cs index ef032ea7791..d6b77afe1d9 100644 --- a/src/Build/BackEnd/BuildManager/BuildManager.cs +++ b/src/Build/BackEnd/BuildManager/BuildManager.cs @@ -519,6 +519,8 @@ ILoggingService InitializeLoggingService() void InitializeCaches() { + Debug.Assert(Monitor.IsEntered(_syncLock)); + var usesInputCaches = _buildParameters.UsesInputCaches(); if (usesInputCaches) @@ -562,6 +564,8 @@ private void InitializeProjectCacheService( ProjectCacheDescriptor pluginDescriptor, CancellationToken cancellationToken) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + if (_projectCacheService != null) { ErrorUtilities.ThrowInternalError("Only one project cache plugin may be set on the BuildManager during a begin / end build session"); @@ -1221,6 +1225,7 @@ CacheResult QueryCache(BuildSubmission buildSubmission, BuildRequestConfiguratio catch { // Set to null so that EndBuild does not try to shut it down and thus rethrow the exception. + Debug.Assert(Monitor.IsEntered(_syncLock)); _projectCacheService = null; throw; } @@ -1272,6 +1277,8 @@ private void AutomaticallyDetectAndInstantiateProjectCacheServiceForVisualStudio BuildSubmission submission, BuildRequestConfiguration config) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + if (BuildEnvironmentHelper.Instance.RunningInVisualStudio && ProjectCacheItems.Count > 0 && !_projectCacheServiceInstantiatedByVSWorkaround && @@ -1412,6 +1419,8 @@ private void LoadSubmissionProjectIntoConfiguration(BuildSubmission submission, /// private void LoadSolutionIntoConfiguration(BuildRequestConfiguration config, BuildRequest request) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + if (config.IsLoaded) { // We've already processed it, nothing to do. @@ -1597,16 +1606,19 @@ private void HandleExecuteSubmissionException(BuildSubmission submission, Except } } - // BuildRequest may be null if the submission fails early on. - if (submission.BuildRequest != null) + lock(_syncLock) { - var result = new BuildResult(submission.BuildRequest, ex); - submission.CompleteResults(result); - submission.CompleteLogging(true); - } + // BuildRequest may be null if the submission fails early on. + if (submission.BuildRequest != null) + { + var result = new BuildResult(submission.BuildRequest, ex); + submission.CompleteResults(result); + submission.CompleteLogging(true); + } - _overallBuildSuccess = false; - CheckSubmissionCompletenessAndRemove(submission); + _overallBuildSuccess = false; + CheckSubmissionCompletenessAndRemove(submission); + } } /// @@ -1628,13 +1640,16 @@ private void HandleExecuteSubmissionException(GraphBuildSubmission submission, E ? ae.InnerExceptions.First() : ex; - if (submission.IsStarted) + lock (_syncLock) { - submission.CompleteResults(new GraphBuildResult(submission.SubmissionId, ex)); - } + if (submission.IsStarted) + { + submission.CompleteResults(new GraphBuildResult(submission.SubmissionId, ex)); + } - _overallBuildSuccess = false; - CheckSubmissionCompletenessAndRemove(submission); + _overallBuildSuccess = false; + CheckSubmissionCompletenessAndRemove(submission); + } } /// @@ -1684,20 +1699,24 @@ private void IssueBuildSubmissionToScheduler(BuildSubmission submission, bool al throw; } - if (resetMainThreadOnFailure) + lock (_syncLock) { - _legacyThreadingData.MainThreadSubmissionId = -1; - } - if (projectException == null) - { - BuildEventContext buildEventContext = new BuildEventContext(submission.SubmissionId, 1, BuildEventContext.InvalidProjectInstanceId, BuildEventContext.InvalidProjectContextId, BuildEventContext.InvalidTargetId, BuildEventContext.InvalidTaskId); - ((IBuildComponentHost)this).LoggingService.LogFatalBuildError(buildEventContext, ex, new BuildEventFileInfo(submission.BuildRequestData.ProjectFullPath)); - } + if (resetMainThreadOnFailure) + { + _legacyThreadingData.MainThreadSubmissionId = -1; + } - submission.CompleteLogging(true); - ReportResultsToSubmission(new BuildResult(submission.BuildRequest, ex)); - _overallBuildSuccess = false; + if (projectException == null) + { + BuildEventContext buildEventContext = new BuildEventContext(submission.SubmissionId, 1, BuildEventContext.InvalidProjectInstanceId, BuildEventContext.InvalidProjectContextId, BuildEventContext.InvalidTargetId, BuildEventContext.InvalidTaskId); + ((IBuildComponentHost)this).LoggingService.LogFatalBuildError(buildEventContext, ex, new BuildEventFileInfo(submission.BuildRequestData.ProjectFullPath)); + } + + submission.CompleteLogging(true); + ReportResultsToSubmission(new BuildResult(submission.BuildRequest, ex)); + _overallBuildSuccess = false; + } } } @@ -1824,7 +1843,10 @@ private void ExecuteGraphBuildScheduler(GraphBuildSubmission submission) ReportResultsToSubmission(result); - _overallBuildSuccess = false; + lock (_syncLock) + { + _overallBuildSuccess = false; + } } } @@ -2016,17 +2038,20 @@ public void Dispose() /// private void ShutdownConnectedNodes(bool abort) { - _shuttingDown = true; + lock (_syncLock) + { + _shuttingDown = true; - // If we are aborting, we will NOT reuse the nodes because their state may be compromised by attempts to shut down while the build is in-progress. - _nodeManager.ShutdownConnectedNodes(!abort && _buildParameters.EnableNodeReuse); + // If we are aborting, we will NOT reuse the nodes because their state may be compromised by attempts to shut down while the build is in-progress. + _nodeManager.ShutdownConnectedNodes(!abort && _buildParameters.EnableNodeReuse); - // if we are aborting, the task host will hear about it in time through the task building infrastructure; - // so only shut down the task host nodes if we're shutting down tidily (in which case, it is assumed that all - // tasks are finished building and thus that there's no risk of a race between the two shutdown pathways). - if (!abort) - { - _taskHostNodeManager.ShutdownConnectedNodes(_buildParameters.EnableNodeReuse); + // if we are aborting, the task host will hear about it in time through the task building infrastructure; + // so only shut down the task host nodes if we're shutting down tidily (in which case, it is assumed that all + // tasks are finished building and thus that there's no risk of a race between the two shutdown pathways). + if (!abort) + { + _taskHostNodeManager.ShutdownConnectedNodes(_buildParameters.EnableNodeReuse); + } } } @@ -2141,6 +2166,8 @@ private int GetNewConfigurationId() /// private BuildRequestConfiguration ResolveConfiguration(BuildRequestConfiguration unresolvedConfiguration, BuildRequestConfiguration matchingConfigurationFromCache, bool replaceProjectInstance) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + BuildRequestConfiguration resolvedConfiguration = matchingConfigurationFromCache ?? _configCache.GetMatchingConfiguration(unresolvedConfiguration); if (resolvedConfiguration == null) { @@ -2172,12 +2199,16 @@ private BuildRequestConfiguration ResolveConfiguration(BuildRequestConfiguration private void ReplaceExistingProjectInstance(BuildRequestConfiguration newConfiguration, BuildRequestConfiguration existingConfiguration) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + existingConfiguration.Project = newConfiguration.Project; _resultsCache.ClearResultsForConfiguration(existingConfiguration.ConfigurationId); } private BuildRequestConfiguration AddNewConfiguration(BuildRequestConfiguration unresolvedConfiguration) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + var newConfigurationId = _scheduler.GetConfigurationIdFromPlan(unresolvedConfiguration.ProjectFullPath); if (_configCache.HasConfiguration(newConfigurationId) || (newConfigurationId == BuildRequestConfiguration.InvalidConfigurationId)) @@ -2232,6 +2263,8 @@ private void HandleNewRequest(int node, BuildRequestBlocker blocker) /// private void HandleResourceRequest(int node, ResourceRequest request) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + if (request.IsResourceAcquire) { // Resource request requires a response and may be blocking. Our continuation is effectively a callback @@ -2256,6 +2289,8 @@ private void HandleResourceRequest(int node, ResourceRequest request) /// private void HandleConfigurationRequest(int node, BuildRequestConfiguration unresolvedConfiguration) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + BuildRequestConfiguration resolvedConfiguration = ResolveConfiguration(unresolvedConfiguration, null, false); var response = new BuildRequestConfigurationResponse(unresolvedConfiguration.ConfigurationId, resolvedConfiguration.ConfigurationId, resolvedConfiguration.ResultsNodeId); @@ -2304,6 +2339,8 @@ private void HandleResult(int node, BuildResult result) /// private void HandleNodeShutdown(int node, NodeShutdown shutdownPacket) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + _shuttingDown = true; ErrorUtilities.VerifyThrow(_activeNodes.Contains(node), "Unexpected shutdown from node {0} which shouldn't exist.", node); _activeNodes.Remove(node); @@ -2366,6 +2403,8 @@ private void HandleNodeShutdown(int node, NodeShutdown shutdownPacket) /// private void CheckForActiveNodesAndCleanUpSubmissions() { + Debug.Assert(Monitor.IsEntered(_syncLock)); + if (_activeNodes.Count == 0) { var submissions = new List(_buildSubmissions.Values); @@ -2416,6 +2455,8 @@ private void CheckForActiveNodesAndCleanUpSubmissions() /// private void PerformSchedulingActions(IEnumerable responses) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + foreach (ScheduleResponse response in responses) { switch (response.Action) @@ -2587,6 +2628,8 @@ private void CheckSubmissionCompletenessAndRemove(GraphBuildSubmission submissio private void CheckAllSubmissionsComplete(BuildRequestDataFlags? flags) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + if (_buildSubmissions.Count == 0 && _graphBuildSubmissions.Count == 0) { if (flags.HasValue && flags.Value.HasFlag(BuildRequestDataFlags.ClearCachesAfterBuild)) @@ -2611,6 +2654,8 @@ private void CheckAllSubmissionsComplete(BuildRequestDataFlags? flags) /// private NodeConfiguration GetNodeConfiguration() { + Debug.Assert(Monitor.IsEntered(_syncLock)); + if (_nodeConfiguration == null) { // Get the remote loggers @@ -2721,9 +2766,12 @@ private void OnProjectFinished(object sender, ProjectFinishedEventArgs e) /// private void OnProjectStarted(object sender, ProjectStartedEventArgs e) { - if (!_projectStartedEvents.ContainsKey(e.BuildEventContext.SubmissionId)) + lock (_syncLock) { - _projectStartedEvents[e.BuildEventContext.SubmissionId] = e; + if (!_projectStartedEvents.ContainsKey(e.BuildEventContext.SubmissionId)) + { + _projectStartedEvents[e.BuildEventContext.SubmissionId] = e; + } } } @@ -2732,6 +2780,8 @@ private void OnProjectStarted(object sender, ProjectStartedEventArgs e) /// private ILoggingService CreateLoggingService(IEnumerable loggers, IEnumerable forwardingLoggers, ISet warningsAsErrors, ISet warningsAsMessages) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + int cpuCount = _buildParameters.MaxNodeCount; LoggerMode loggerMode = cpuCount == 1 && _buildParameters.UseSynchronousLogging @@ -2899,6 +2949,8 @@ private void Dispose(bool disposing) private bool ReuseOldCaches(string[] inputCacheFiles) { + Debug.Assert(Monitor.IsEntered(_syncLock)); + ErrorUtilities.VerifyThrowInternalNull(inputCacheFiles, nameof(inputCacheFiles)); ErrorUtilities.VerifyThrow(_configCache == null, "caches must not be set at this point"); ErrorUtilities.VerifyThrow(_resultsCache == null, "caches must not be set at this point"); @@ -2978,6 +3030,8 @@ private void LogErrorAndShutdown(string message) private void CancelAndMarkAsFailure() { + Debug.Assert(Monitor.IsEntered(_syncLock)); + CancelAllSubmissions(); // CancelAllSubmissions also ends up setting _shuttingDown and _overallBuildSuccess but it does so in a separate thread to avoid deadlocks. diff --git a/src/Build/Construction/ProjectRootElement.cs b/src/Build/Construction/ProjectRootElement.cs index 9d86a4731dd..cd19bb38ebc 100644 --- a/src/Build/Construction/ProjectRootElement.cs +++ b/src/Build/Construction/ProjectRootElement.cs @@ -1677,19 +1677,33 @@ private void ReloadFrom(Func documentProducer, bo { ThrowIfUnsavedChanges(throwIfUnsavedChanges); - XmlDocumentWithLocation document = documentProducer(preserveFormatting ?? PreserveFormatting); - - // Reload should only mutate the state if there are no parse errors. - ThrowIfDocumentHasParsingErrors(document); - - // Do not clear the string cache. - // Based on the assumption that Projects are reloaded repeatedly from their file with small increments, - // and thus most strings would get reused - //this.XmlDocument.ClearAnyCachedStrings(); + var oldDocument = XmlDocument; + XmlDocumentWithLocation newDocument = documentProducer(preserveFormatting ?? PreserveFormatting); + try + { + // Reload should only mutate the state if there are no parse errors. + ThrowIfDocumentHasParsingErrors(newDocument); - RemoveAllChildren(); + RemoveAllChildren(); - ProjectParser.Parse(document, this); + ProjectParser.Parse(newDocument, this); + } + finally + { + // Whichever document didn't become this element's document must be removed from the string cache. + // We do it after the fact based on the assumption that Projects are reloaded repeatedly from their + // file with small increments, and thus most strings would get reused avoiding unnecessary churn in + // the string cache. + var currentDocument = XmlDocument; + if (!object.ReferenceEquals(currentDocument, oldDocument)) + { + oldDocument.ClearAnyCachedStrings(); + } + if (!object.ReferenceEquals(currentDocument, newDocument)) + { + newDocument.ClearAnyCachedStrings(); + } + } MarkDirty("Project reloaded", null); } diff --git a/src/Build/Evaluation/ProjectStringCache.cs b/src/Build/Evaluation/ProjectStringCache.cs index 699964700c6..32277f91fec 100644 --- a/src/Build/Evaluation/ProjectStringCache.cs +++ b/src/Build/Evaluation/ProjectStringCache.cs @@ -61,6 +61,20 @@ internal int Count } } + /// + /// Obtain the number of documents contained in the cache. + /// + internal int DocumentCount + { + get + { + lock (_locker) + { + return _documents.Count; + } + } + } + /// /// Add the given string to the cache or return the existing string if it is already /// in the cache. diff --git a/src/Samples/ProjectCachePlugin/AssemblyMockCache.cs b/src/Samples/ProjectCachePlugin/AssemblyMockCache.cs index c89e8c9e3c6..3b26b82d942 100644 --- a/src/Samples/ProjectCachePlugin/AssemblyMockCache.cs +++ b/src/Samples/ProjectCachePlugin/AssemblyMockCache.cs @@ -54,6 +54,7 @@ private static void ErrorFrom(string errorLocation, PluginLoggerBase pluginLogge switch (errorKind) { case "Exception": + pluginLoggerBase?.LogMessage($"{errorLocation} is going to throw an exception", MessageImportance.High); throw new Exception($"Cache plugin exception from {errorLocation}"); case "LoggedError": pluginLoggerBase?.LogError($"Cache plugin logged error from {errorLocation}"); diff --git a/src/Shared/TranslatorHelpers.cs b/src/Shared/TranslatorHelpers.cs index 9cab3485c97..c3e7d12b6ad 100644 --- a/src/Shared/TranslatorHelpers.cs +++ b/src/Shared/TranslatorHelpers.cs @@ -261,9 +261,6 @@ public static void Translate(this ITranslator translator, ref AssemblyName assem HashAlgorithm = hashAlgorithm, VersionCompatibility = versionCompatibility, CodeBase = codeBase, - // AssemblyName.KeyPair is not used anywhere, additionally StrongNameKeyPair is not supported in .net core 5- - // and throws platform not supported exception when serialized or deserialized - KeyPair = null, }; assemblyName.SetPublicKey(publicKey); diff --git a/src/Tasks/StateFileBase.cs b/src/Tasks/StateFileBase.cs index f228e84bd07..29f696307ca 100644 --- a/src/Tasks/StateFileBase.cs +++ b/src/Tasks/StateFileBase.cs @@ -72,7 +72,7 @@ internal static StateFileBase DeserializeCache(string stateFile, TaskLoggingHelp { if (!string.IsNullOrEmpty(stateFile) && FileSystems.Default.FileExists(stateFile)) { - using (FileStream s = new FileStream(stateFile, FileMode.Open)) + using (FileStream s = File.OpenRead(stateFile)) { var translator = BinaryTranslator.GetReadTranslator(s, buffer: null); byte version = 0;