diff --git a/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs b/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs index 86fc2b49bb1..1f66abcb633 100644 --- a/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs +++ b/src/Build.UnitTests/BackEnd/BuildRequestConfiguration_Tests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Threading.Tasks; using System.Xml; using Microsoft.Build.BackEnd; using Microsoft.Build.Collections; @@ -674,6 +675,70 @@ public void TestProjectEvaluationIdPreservedInShallowClone() clone.ProjectEvaluationId.ShouldBe(expectedEvalId); } + [Fact] + public async Task CacheIfPossible_WhileProjectInstanceUsageIsHeld_DoesNotCache() + { + string projectBody = """ + + + + """.Cleanup(); + + ProjectCollection collection = _env.CreateProjectCollection().Collection; + using ProjectFromString projectFromString = new( + projectBody, + new Dictionary(), + ObjectModelHelpers.MSBuildDefaultToolsVersion, + collection); + Project project = projectFromString.Project; + project.FullPath = "foo"; + ProjectInstance instance = project.CreateProjectInstance(); + + BuildRequestConfiguration configuration = new(new BuildRequestData(instance, [], null), "2.0") + { + ConfigurationId = 1, + IsCacheable = true, + }; + + _env.WithTransientTestState(new TransientConfigurationCacheFile(configuration)); + + configuration.CacheIfPossible(); + configuration.IsCached.ShouldBeTrue(); + + using (configuration.AcquireProjectInstanceUsage()) + { + configuration.IsCached.ShouldBeFalse(); + + using (configuration.AcquireProjectInstanceUsage()) + { + await Task.Run(configuration.CacheIfPossible); + configuration.IsCached.ShouldBeFalse(); + + // Caching nulls these out, so assert the project state itself and not just the flag. + instance.GlobalPropertiesDictionary.ShouldNotBeNull(); + instance.PropertiesToBuildWith.ShouldNotBeNull(); + instance.ItemsToBuildWith.ShouldNotBeNull(); + } + + configuration.CacheIfPossible(); + configuration.IsCached.ShouldBeFalse(); + } + + configuration.CacheIfPossible(); + configuration.IsCached.ShouldBeTrue(); + } + + private sealed class TransientConfigurationCacheFile : TransientTestState + { + private readonly BuildRequestConfiguration _configuration; + + internal TransientConfigurationCacheFile(BuildRequestConfiguration configuration) + { + _configuration = configuration; + } + + public override void Revert() => _configuration.ClearCacheFile(); + } [Fact] public void TestProjectEvaluationIdPreservedAcrossTranslateForFutureUse() diff --git a/src/Build.UnitTests/BackEnd/RequestBuilder_Tests.cs b/src/Build.UnitTests/BackEnd/RequestBuilder_Tests.cs index 5050aa04044..09dc0683632 100644 --- a/src/Build.UnitTests/BackEnd/RequestBuilder_Tests.cs +++ b/src/Build.UnitTests/BackEnd/RequestBuilder_Tests.cs @@ -246,6 +246,48 @@ public void TestMissingProjectFile() Assert.Equal(typeof(InvalidProjectFileException), _buildRequestCompleted_Entry.Result.Exception.GetType()); } + [Fact] + public void BuildProject_ConfigurationCacheSweepDuringBuild_DoesNotCacheConfiguration() + { + BuildRequestConfiguration configuration = CreateTestProject(1); + try + { + TestTargetBuilder targetBuilder = (TestTargetBuilder)_host.GetComponent(BuildComponentType.TargetBuilder); + IConfigCache configCache = (IConfigCache)_host.GetComponent(BuildComponentType.ConfigCache); + + configCache.AddConfiguration(configuration); + + BuildRequest request = CreateNewBuildRequest(1, new string[1] { "target1" }); + BuildRequestEntry entry = new BuildRequestEntry(request, configuration, CreateStubTaskEnvironment()); + BuildResult result = new BuildResult(request); + result.AddResultsForTarget("target1", GetEmptySuccessfulTargetResult()); + targetBuilder.SetResultsToReturn(result); + + bool? cachedDuringBuild = null; + + // Simulate the BuildRequestEngine memory pressure sweep running on another thread while + // the request builder is still using the project instance of the configuration it builds. + targetBuilder.ActionBeforeReturningResult = () => + { + Task.Run(() => configCache.WriteConfigurationsToDisk()).GetAwaiter().GetResult(); + cachedDuringBuild = configuration.IsCached; + }; + + _requestBuilder.BuildRequest(GetNodeLoggingContext(), entry); + + WaitForEvent(_buildRequestCompletedEvent, "Build Request Completed"); + + cachedDuringBuild.ShouldBe(false); + _buildRequestCompleted_Entry.Result.Exception.ShouldBeNull(); + Assert.Equal(BuildResultCode.Success, _buildRequestCompleted_Entry.Result.OverallResult); + } + finally + { + configuration.ClearCacheFile(); + DeleteTestProject(configuration); + } + } + [Fact] public void RequestThreadProcEventsIncludeRequestContext() { @@ -500,6 +542,12 @@ internal void SetResultsToReturn(BuildResult result) _cache.AddResult(result); } + /// + /// Invoked right before the cached result is returned, allowing tests to observe the state + /// of the build while the request builder is still building the project. + /// + internal Action ActionBeforeReturningResult { get; set; } + internal void SetNewBuildRequests(FullyQualifiedBuildRequest[] requests) { _newRequests = requests; @@ -547,8 +595,9 @@ public Task BuildTargets(ProjectLoggingContext loggingContext, Buil } } - return Task.FromResult(_cache.GetResultForRequest(entry.Request)); - } + ActionBeforeReturningResult?.Invoke(); + + return Task.FromResult(_cache.GetResultForRequest(entry.Request)); } #endregion diff --git a/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs b/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs index 1113335b0cf..a3f01d6a868 100644 --- a/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs +++ b/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs @@ -382,10 +382,11 @@ public void SubmitBuildRequest(BuildRequest request) BuildResult resultToReport = new BuildResult(request, result, null); BuildRequestConfiguration config = ((IConfigCache)_componentHost.GetComponent(BuildComponentType.ConfigCache))[request.ConfigurationId]; - // Retrieve the config if it has been cached, since this would contain our instance data. It is safe to do this outside of a lock - // since only one thread can run in the BuildRequestEngine at a time, and it is EvaluateRequestStates which would cause us to - // cache configurations if we are running out of memory. - config.RetrieveFromCache(); + // Retrieve the project if it is cached and keep it in memory while preparing the result for + // transfer. Multiple in-proc request engines share the configuration cache, so another engine + // may run a memory-pressure sweep concurrently. + using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage = + config.AcquireProjectInstanceUsage(); ((IBuildResults)resultToReport).SavedCurrentDirectory = config.SavedCurrentDirectory; ((IBuildResults)resultToReport).SavedEnvironmentVariables = config.SavedEnvironmentVariables; if ((request.BuildRequestDataFlags & BuildRequestDataFlags.IgnoreExistingProjectState) != BuildRequestDataFlags.IgnoreExistingProjectState) diff --git a/src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs b/src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs index 8e09004f299..e2dec115ab2 100644 --- a/src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs +++ b/src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs @@ -490,11 +490,9 @@ public bool ShouldUseCache(BuildRequestConfiguration buildRequestConfiguration) return false; } - // We need to retrieve the configuration if it's already loaded in order to access the Project property below. - if (buildRequestConfiguration.IsCached) - { - buildRequestConfiguration.RetrieveFromCache(); - } + // Retrieve the project if it is cached and keep it in memory while checking its cache descriptors. + using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage = + buildRequestConfiguration.AcquireProjectInstanceUsage(); // Check if there are any project cache items defined in the project return GetProjectCacheDescriptors(buildRequestConfiguration.Project).Any(); @@ -530,6 +528,12 @@ public void PostCacheRequest(CacheRequest cacheRequest, CancellationToken cancel async ValueTask<(CacheResult Result, int ProjectContextId)> ProcessCacheRequestAsync() { + // The configuration cache is shared with the in-proc node's BuildRequestEngine, which may run a + // memory-pressure sweep on its own thread. Keep the project in memory for this entire operation: + // the ProjectInstance below is handed to the plugin and is used until the query completes. + using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage = + cacheRequest.Configuration.AcquireProjectInstanceUsage(); + EvaluateProjectIfNecessary(cacheRequest.Submission, cacheRequest.Configuration); BuildRequestData buildRequest = new BuildRequestData( @@ -839,15 +843,13 @@ public async Task HandleBuildResultAsync( return; } - // If the ProjectInstance was evicted to disk to save memory, restore it before accessing - // the Project property below (whose getter asserts !IsCached). - if (requestConfiguration.IsCached) + List projectCacheDescriptors; + // Retrieve the project if it is cached and keep it in memory while determining which plugins apply. + using (requestConfiguration.AcquireProjectInstanceUsage()) { - requestConfiguration.RetrieveFromCache(); + projectCacheDescriptors = GetProjectCacheDescriptors(requestConfiguration.Project).ToList(); } - // Filter to plugins which apply to the project, if any - List projectCacheDescriptors = GetProjectCacheDescriptors(requestConfiguration.Project).ToList(); if (projectCacheDescriptors.Count == 0) { return; diff --git a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs index b8c8f7308da..f2d7f419c40 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs @@ -1152,11 +1152,16 @@ private async Task BuildProject() buildCheckManager?.SetDataSource(BuildCheckDataSource.BuildExecution); - // Make sure it is null before loading the configuration into the request, because if there is a problem - // we do not wand to have an invalid projectLoggingContext floating around. Also if this is null the error will be - // logged with the node logging context + // Make sure it is null before loading or restoring the configuration so an invalid context is not reused. + // The failure paths below create a temporary project logging context before propagating the error. _projectLoggingContext = null; + // The configuration cache is shared across the node and may be swept for memory pressure on a + // BuildRequestEngine thread. A configuration becomes cacheable after its last target, but post-build + // telemetry still accesses its ProjectInstance, so keep the project in memory for this entire operation. + using BuildRequestConfiguration.ProjectInstanceUsageScope projectInstanceUsage = + AcquireProjectInstanceUsageWithProjectLoggingContext(); + try { // Load the project @@ -1192,11 +1197,7 @@ private async Task BuildProject() catch { // make sure that any errors thrown by a child project are logged in the context of their parent project: create a temporary projectLoggingContext - _projectLoggingContext = new ProjectLoggingContext( - _nodeLoggingContext, - _requestEntry.Request, - _requestEntry.RequestConfiguration.ProjectFullPath, - _requestEntry.RequestConfiguration.ToolsVersion); + CreateTemporaryProjectLoggingContext(); throw; } @@ -1211,7 +1212,7 @@ private async Task BuildProject() { // Determine the set of targets we need to build (string name, TargetBuiltReason reason)[] allTargets = _requestEntry.RequestConfiguration - .GetTargetsUsedToBuildRequest(_requestEntry.Request).ToArray(); + .GetTargetsUsedToBuildRequest(_requestEntry.Request).ToArray(); if (MSBuildEventSource.Log.IsEnabled()) { MSBuildEventSource.Log.BuildProjectStart(_requestEntry.RequestConfiguration.ProjectFullPath, string.Join(", ", allTargets)); @@ -1221,8 +1222,6 @@ private async Task BuildProject() // Make sure to extract known immutable folders from properties and register them for fast up-to-date check ConfigureKnownImmutableFolders(); - // See comment on Microsoft.Build.Internal.Utilities.GenerateToolsVersionToUse - _requestEntry.RequestConfiguration.RetrieveFromCache(); if (_requestEntry.RequestConfiguration.Project.UsingDifferentToolsVersionFromProjectFile) { _projectLoggingContext.LogComment(MessageImportance.Low, @@ -1314,6 +1313,28 @@ BuildResult CopyTargetResultsFromProxyTargetsToRealTargets(BuildResult resultFro } } + private BuildRequestConfiguration.ProjectInstanceUsageScope AcquireProjectInstanceUsageWithProjectLoggingContext() + { + try + { + return _requestEntry.RequestConfiguration.AcquireProjectInstanceUsage(); + } + catch + { + CreateTemporaryProjectLoggingContext(); + throw; + } + } + + private void CreateTemporaryProjectLoggingContext() + { + _projectLoggingContext = new ProjectLoggingContext( + _nodeLoggingContext, + _requestEntry.Request, + _requestEntry.RequestConfiguration.ProjectFullPath, + _requestEntry.RequestConfiguration.ToolsVersion); + } + private void UpdateStatisticsPostBuild() { ITelemetryCollector telemetryCollector = _nodeLoggingContext?.TelemetryCollector; diff --git a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs index 50cd513c512..5ed219b6fde 100644 --- a/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs +++ b/src/Build/BackEnd/Shared/BuildRequestConfiguration.cs @@ -125,6 +125,11 @@ internal class BuildRequestConfiguration : IEquatable /// private Dictionary _activelyBuildingTargets; + /// + /// The number of operations currently using the in-memory project state. + /// + private int _projectInstanceUsageCount; + /// /// The node where this configuration's master results are stored. /// @@ -639,6 +644,48 @@ public Lookup BaseLookup public Dictionary ActivelyBuildingTargets => _activelyBuildingTargets ?? (_activelyBuildingTargets = new Dictionary(StringComparer.OrdinalIgnoreCase)); + /// + /// Keeps the in memory while the caller uses it, preventing a concurrent + /// memory-pressure cache sweep. Retrieves the project first if it was already cached. + /// + /// + /// The usage count belongs to this configuration. A shallow clone that shares the same + /// has independent synchronization and usage tracking. + /// + internal ProjectInstanceUsageScope AcquireProjectInstanceUsage() => new(this); + + /// + /// Tracks one active consumer. Consume with ; + /// do not dispose copies. + /// + internal readonly struct ProjectInstanceUsageScope : IDisposable + { + private readonly BuildRequestConfiguration _configuration; + + internal ProjectInstanceUsageScope(BuildRequestConfiguration configuration) + { + _configuration = configuration; + + lock (_configuration._syncLock) + { + _configuration.RetrieveFromCache(); + Assumed.False(_configuration.IsCached, "Configuration could not be retrieved before accessing the project."); + _configuration._projectInstanceUsageCount++; + } + } + + public void Dispose() + { + Assumed.NotNull(_configuration, "ProjectInstance usage scope was not initialized."); + + lock (_configuration._syncLock) + { + Assumed.True(_configuration._projectInstanceUsageCount > 0, "No active ProjectInstance usage to complete."); + _configuration._projectInstanceUsageCount--; + } + } + } + /// /// Holds a snapshot of the environment at the time we blocked. /// @@ -725,7 +772,7 @@ public void CacheIfPossible() { lock (_syncLock) { - if (IsActivelyBuilding || IsCached || !IsLoaded || !IsCacheable) + if (_projectInstanceUsageCount > 0 || IsActivelyBuilding || IsCached || !IsLoaded || !IsCacheable) { return; }