Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock writes to _overallBuildSuccess #6412

Merged
merged 4 commits into from
May 24, 2021
Merged

Lock writes to _overallBuildSuccess #6412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52d8a4c
Lock writes to _overallBuildSuccess
cdmihai May 4, 2021
55687ee
Log when mock cache is about to throw an exception
cdmihai May 4, 2021
ab28a0f
More locking
cdmihai May 5, 2021
14fb892
Debug assert that lock is held
cdmihai May 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 90 additions & 36 deletions src/Build/BackEnd/BuildManager/BuildManager.cs
View file
Open in desktop
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;
cdmihai marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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