Skip to content

Commit

Permalink
Lock writes to _overallBuildSuccess (#6412)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@cdmihai
cdmihai authored May 24, 2021
1 parent 14572cb commit bc90887
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 36 deletions.
126 changes: 90 additions & 36 deletions src/Build/BackEnd/BuildManager/BuildManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,8 @@ ILoggingService InitializeLoggingService()

void InitializeCaches()
{
Debug.Assert(Monitor.IsEntered(_syncLock));

var usesInputCaches = _buildParameters.UsesInputCaches();

if (usesInputCaches)
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -1272,6 +1277,8 @@ private void AutomaticallyDetectAndInstantiateProjectCacheServiceForVisualStudio
BuildSubmission submission,
BuildRequestConfiguration config)
{
Debug.Assert(Monitor.IsEntered(_syncLock));

if (BuildEnvironmentHelper.Instance.RunningInVisualStudio &&
ProjectCacheItems.Count > 0 &&
!_projectCacheServiceInstantiatedByVSWorkaround &&
Expand Down Expand Up @@ -1412,6 +1419,8 @@ private void LoadSubmissionProjectIntoConfiguration(BuildSubmission submission,
/// </summary>
private void LoadSolutionIntoConfiguration(BuildRequestConfiguration config, BuildRequest request)
{
Debug.Assert(Monitor.IsEntered(_syncLock));

if (config.IsLoaded)
{
// We've already processed it, nothing to do.
Expand Down Expand Up @@ -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);
}
}

/// <summary>
Expand All @@ -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);
}
}

/// <summary>
Expand Down Expand Up @@ -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;
}
}
}

Expand Down Expand Up @@ -1824,7 +1843,10 @@ private void ExecuteGraphBuildScheduler(GraphBuildSubmission submission)

ReportResultsToSubmission(result);

_overallBuildSuccess = false;
lock (_syncLock)
{
_overallBuildSuccess = false;
}
}
}

Expand Down Expand Up @@ -2016,17 +2038,20 @@ public void Dispose()
/// </summary>
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);
}
}
}

Expand Down Expand Up @@ -2141,6 +2166,8 @@ private int GetNewConfigurationId()
/// </summary>
private BuildRequestConfiguration ResolveConfiguration(BuildRequestConfiguration unresolvedConfiguration, BuildRequestConfiguration matchingConfigurationFromCache, bool replaceProjectInstance)
{
Debug.Assert(Monitor.IsEntered(_syncLock));

BuildRequestConfiguration resolvedConfiguration = matchingConfigurationFromCache ?? _configCache.GetMatchingConfiguration(unresolvedConfiguration);
if (resolvedConfiguration == null)
{
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -2232,6 +2263,8 @@ private void HandleNewRequest(int node, BuildRequestBlocker blocker)
/// </summary>
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
Expand All @@ -2256,6 +2289,8 @@ private void HandleResourceRequest(int node, ResourceRequest request)
/// </summary>
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);
Expand Down Expand Up @@ -2304,6 +2339,8 @@ private void HandleResult(int node, BuildResult result)
/// </summary>
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);
Expand Down Expand Up @@ -2366,6 +2403,8 @@ private void HandleNodeShutdown(int node, NodeShutdown shutdownPacket)
/// </remarks>
private void CheckForActiveNodesAndCleanUpSubmissions()
{
Debug.Assert(Monitor.IsEntered(_syncLock));

if (_activeNodes.Count == 0)
{
var submissions = new List<BuildSubmission>(_buildSubmissions.Values);
Expand Down Expand Up @@ -2416,6 +2455,8 @@ private void CheckForActiveNodesAndCleanUpSubmissions()
/// </summary>
private void PerformSchedulingActions(IEnumerable<ScheduleResponse> responses)
{
Debug.Assert(Monitor.IsEntered(_syncLock));

foreach (ScheduleResponse response in responses)
{
switch (response.Action)
Expand Down Expand Up @@ -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))
Expand All @@ -2611,6 +2654,8 @@ private void CheckAllSubmissionsComplete(BuildRequestDataFlags? flags)
/// </summary>
private NodeConfiguration GetNodeConfiguration()
{
Debug.Assert(Monitor.IsEntered(_syncLock));

if (_nodeConfiguration == null)
{
// Get the remote loggers
Expand Down Expand Up @@ -2721,9 +2766,12 @@ private void OnProjectFinished(object sender, ProjectFinishedEventArgs e)
/// </summary>
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;
}
}
}

Expand All @@ -2732,6 +2780,8 @@ private void OnProjectStarted(object sender, ProjectStartedEventArgs e)
/// </summary>
private ILoggingService CreateLoggingService(IEnumerable<ILogger> loggers, IEnumerable<ForwardingLoggerRecord> forwardingLoggers, ISet<string> warningsAsErrors, ISet<string> warningsAsMessages)
{
Debug.Assert(Monitor.IsEntered(_syncLock));

int cpuCount = _buildParameters.MaxNodeCount;

LoggerMode loggerMode = cpuCount == 1 && _buildParameters.UseSynchronousLogging
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions src/Samples/ProjectCachePlugin/AssemblyMockCache.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
Expand Down

0 comments on commit bc90887

Please sign in to comment.