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

[automated] Merge branch 'vs16.11' => 'main' #6476

Merged
merged 8 commits into from
Jun 1, 2021
Merged

[automated] Merge branch 'vs16.11' => 'main' #6476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a1e93f1
Fix XmlDocument leaks when reloading projects
ladipro May 14, 2021
d178962
Add new test ReloadDoesNotLeakCachedXmlDocuments
ladipro May 14, 2021
14572cb
Merge pull request #6457 from ladipro/fix-xml-reload-leak
Forgind May 24, 2021
bc90887
Lock writes to _overallBuildSuccess (#6412)
cdmihai May 24, 2021
0d1f270
Use packaged C# compiler (#6431)
rainersigwald May 24, 2021
1c6e7ad
Switch to File.OpenRead (#6393)
Forgind May 24, 2021
c1d3185
Remove KeyPair deserialization (#6395)
Forgind May 24, 2021
b396727
Merge branch 'main' into merge/vs16.11-to-main
Forgind May 24, 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
1 change: 1 addition & 0 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<!-- Repo Toolset Features -->
<PropertyGroup Condition="'$(MonoBuild)' != 'true'">
<UsingToolIbcOptimization>true</UsingToolIbcOptimization>
<UsingToolMicrosoftNetCompilers>true</UsingToolMicrosoftNetCompilers><!-- Force a specific compiler version because record changes cause genapi output to flip-flop -->
<UsingToolVisualStudioIbcTraining>true</UsingToolVisualStudioIbcTraining>
<UsingToolSymbolUploader>true</UsingToolSymbolUploader>
<UsingToolVSSDK>true</UsingToolVSSDK>
Expand Down
39 changes: 39 additions & 0 deletions src/Build.OM.UnitTests/Construction/ProjectRootElement_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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("<XXX />"); // Invalid root element
}
Should.Throw<InvalidProjectFileException>(() => projectElement.Reload(false));
GetNumberOfDocumentsInProjectStringCache(projectElement).ShouldBe(originalDocumentCount);
}

private void AssertReload(
string initialContents,
string changedContents,
Expand Down Expand Up @@ -1986,5 +2013,17 @@ private void VerifyAssertLineByLine(string expected, string actual)
{
Helpers.VerifyAssertLineByLine(expected, actual, false);
}

/// <summary>
/// Returns the number of documents retained by the project string cache.
/// Peeks at it via reflection since internals are not visible to these tests.
/// </summary>
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>());
object cache = document.GetType().InvokeMember("StringCache", bindingFlags, null, document, Array.Empty<object>());
return (int)cache.GetType().InvokeMember("DocumentCount", bindingFlags, null, cache, Array.Empty<object>());
}
}
}
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;
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
Loading