From bc90887cf4f0e19c3d14c1167136acd072616cc3 Mon Sep 17 00:00:00 2001 From: Mihai Codoban Date: Mon, 24 May 2021 08:20:45 -0700 Subject: [PATCH] Lock writes to _overallBuildSuccess (#6412) Fixes https://teams.microsoft.com/l/message/19:3212bf033f4c4b5198643a04fa1048fa@thread.skype/1620160142660?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=4ba7372f-2799-4677-89f0-7a1aaea3706c&parentMessageId=1620160142660&teamName=.NET%20Developer%20Experience&channelName=MSBuild&createdTime=1620160142660 Changes Made Protected writes to _overallBuildSuccess with _syncLock --- .../BackEnd/BuildManager/BuildManager.cs | 126 +++++++++++++----- .../ProjectCachePlugin/AssemblyMockCache.cs | 1 + 2 files changed, 91 insertions(+), 36 deletions(-) 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/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}");