diff --git a/src/Build.UnitTests/Instance/TaskItem_Tests.cs b/src/Build.UnitTests/Instance/TaskItem_Tests.cs index 7e6fd0f64f4..a593b67d63c 100644 --- a/src/Build.UnitTests/Instance/TaskItem_Tests.cs +++ b/src/Build.UnitTests/Instance/TaskItem_Tests.cs @@ -373,5 +373,45 @@ public void Escaping3() logger.AssertLogContains("i1%2ai2"); } + + /// + /// Regression test for the ObjectDisposedException race rooted in returning a + /// `yield return` iterator from EnumerateMetadata over an ImmutableDictionary. + /// See dotnet/runtime#92290 and the EnumerateMetadataEager rationale comment. + /// + /// The intent of the fix is that callers always receive a materialized snapshot + /// (an array), never a compiler-generated state-machine iterator that parks a + /// copy of the ImmutableDictionary.Enumerator struct across yield boundaries. + /// Asserting on the runtime type makes a future revert of the fix observable. + /// + [Fact] + public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator() + { + ProjectItemInstance.TaskItem item = new("foo", "test.proj"); + item.SetMetadata("a", "1"); + item.SetMetadata("b", "2"); + + IEnumerable> result = ((IMetadataContainer)item).EnumerateMetadata(); + + // Type-level invariant: result must be a materialized array, not a compiler-generated + // yield-return state machine that parks a copy of the ImmutableDictionary.Enumerator + // struct across yield boundaries. + result.ShouldBeOfType[]>(); + + // Behavioural invariant: the snapshot must not observe later mutations of the item. + // A streaming iterator would re-enumerate _directMetadata on iteration, exposing + // mutations made after EnumerateMetadata returned. + item.SetMetadata("a", "changed-after-snapshot"); + item.SetMetadata("c", "added-after-snapshot"); + result.ShouldBe(new[] + { + new KeyValuePair("a", "1"), + new KeyValuePair("b", "2"), + }, ignoreOrder: true); + + // No-custom-metadata case short-circuits to Array.Empty<>. + ProjectItemInstance.TaskItem empty = new("bar", "test.proj"); + ((IMetadataContainer)empty).EnumerateMetadata().ShouldBeEmpty(); + } } } diff --git a/src/Build/Instance/ProjectItemInstance.cs b/src/Build/Instance/ProjectItemInstance.cs index fcb166f65f4..cdd17b2e3b7 100644 --- a/src/Build/Instance/ProjectItemInstance.cs +++ b/src/Build/Instance/ProjectItemInstance.cs @@ -1117,24 +1117,7 @@ public IEnumerable> EnumerateMetadata() // If we have item definitions, call the expensive property that does the right thing. // Otherwise use _directMetadata to avoid allocations caused by DeepClone(). var list = _itemDefinitions != null ? MetadataCollection : DirectMetadata; - if (list != null) - { -#if FEATURE_APPDOMAIN - // Can't send a yield-return iterator across AppDomain boundaries - if (!AppDomain.CurrentDomain.IsDefaultAppDomain()) - { - return EnumerateMetadataEager(list); - } -#endif - // Mainline scenario, returns an iterator to avoid allocating an array - // to store the results. With the iterator, results can stream to the - // consumer (e.g. binlog writer) without allocations. - return EnumerateMetadata(list); - } - else - { - return []; - } + return list != null ? EnumerateMetadataEager(list) : []; } /// @@ -1192,33 +1175,39 @@ private void ImportMetadata(IEnumerable> metadata, } } -#if FEATURE_APPDOMAIN /// - /// Used to return metadata from another AppDomain. Can't use yield return because the - /// generated state machine is not marked as [Serializable], so we need to allocate. + /// Materialize the metadata snapshot eagerly into a heap-allocated array. /// + /// + /// Used historically only on the AppDomain-crossing path because a yield-iterator + /// state machine is not [Serializable]. Now used unconditionally to avoid letting + /// a copy of the pooled + /// struct escape the call frame inside a compiler-generated iterator. That state + /// machine has a complex lifetime that has been observed to interact with logger + /// serialization to produce ObjectDisposedException in + /// , surfacing as + /// MSB4166 "Child node exited prematurely" — see dotnet/runtime#92290. Eager + /// materialization removes the entire class of issues by returning a regular + /// array that has no relationship to the BCL enumerator pool. + /// /// The source list to return metadata from. /// An array of string key-value pairs representing metadata. private IEnumerable> EnumerateMetadataEager(ImmutableDictionary list) { - var result = new List>(list.Count); - - foreach (KeyValuePair projectMetadataInstance in list) + int count = list.Count; + if (count == 0) { - result.Add(new KeyValuePair(projectMetadataInstance.Key, EscapingUtilities.UnescapeAll(projectMetadataInstance.Value))); + return []; } - - // Probably better to send the raw array across the wire even if it's another allocation. - return result.ToArray(); - } -#endif - - private IEnumerable> EnumerateMetadata(ImmutableDictionary list) - { + var snapshot = new KeyValuePair[count]; + int i = 0; foreach (KeyValuePair projectMetadataInstance in list) { - yield return new KeyValuePair(projectMetadataInstance.Key, EscapingUtilities.UnescapeAll(projectMetadataInstance.Value)); + snapshot[i++] = new KeyValuePair( + projectMetadataInstance.Key, + EscapingUtilities.UnescapeAll(projectMetadataInstance.Value)); } + return snapshot; } /// diff --git a/src/Utilities.UnitTests/TaskItem_Tests.cs b/src/Utilities.UnitTests/TaskItem_Tests.cs index 6e0cc3a9afa..e8bb9c703f1 100644 --- a/src/Utilities.UnitTests/TaskItem_Tests.cs +++ b/src/Utilities.UnitTests/TaskItem_Tests.cs @@ -342,6 +342,42 @@ public void ImplementsIMetadataContainer() Assert.True(actualMetadata.SequenceEqual(expectedMetadata)); } + /// + /// Regression test for the ObjectDisposedException race rooted in returning a + /// `yield return` iterator from EnumerateMetadata over an ImmutableDictionary. + /// See dotnet/runtime#92290. + /// + /// EnumerateMetadata must return a materialized snapshot (an array), not a + /// compiler-generated state-machine iterator that parks a copy of the + /// ImmutableDictionary.Enumerator struct (and its pooled traversal stack) + /// across yield boundaries. + /// + [Fact] + public void EnumerateMetadata_ReturnsMaterializedSnapshot_NotYieldIterator() + { + TaskItem item = new TaskItem("foo"); + ((IMetadataContainer)item).ImportMetadata(new Dictionary { { "a", "1" }, { "b", "2" } }); + + IEnumerable> result = ((IMetadataContainer)item).EnumerateMetadata(); + + // Type-level invariant: result must be a materialized array, not a compiler-generated + // yield-return state machine that parks a copy of the ImmutableDictionary.Enumerator + // struct across yield boundaries. + result.ShouldBeOfType[]>(); + + // Behavioural invariant: the snapshot must not observe later mutations of the item. + item.SetMetadata("a", "changed-after-snapshot"); + item.SetMetadata("c", "added-after-snapshot"); + result.ShouldBe(new[] + { + new KeyValuePair("a", "1"), + new KeyValuePair("b", "2"), + }, ignoreOrder: true); + + TaskItem empty = new TaskItem("bar"); + ((IMetadataContainer)empty).EnumerateMetadata().ShouldBeEmpty(); + } + #if FEATURE_APPDOMAIN /// /// Test that task items can be successfully constructed based on a task item from another appdomain. diff --git a/src/Utilities/TaskItem.cs b/src/Utilities/TaskItem.cs index c25f4cf6148..0f253c370cc 100644 --- a/src/Utilities/TaskItem.cs +++ b/src/Utilities/TaskItem.cs @@ -539,18 +539,15 @@ IDictionary ITaskItem2.CloneCustomMetadataEscaped() => _metadata == null IEnumerable> IMetadataContainer.EnumerateMetadata() { -#if FEATURE_APPDOMAIN - // Can't send a yield-return iterator across AppDomain boundaries - // so have to allocate - if (!AppDomain.CurrentDomain.IsDefaultAppDomain()) - { - return EnumerateMetadataEager(); - } -#endif - - // In general case we want to return an iterator without allocating a collection - // to hold the result, so we can stream the items directly to the consumer. - return EnumerateMetadataLazy(); + // Always materialize eagerly. A yield-return iterator over an ImmutableDictionary + // parks a copy of the dictionary's pooled enumerator struct inside a heap-allocated + // state machine whose lifetime is harder to reason about than a one-shot snapshot; + // it has been observed to interact with logger serialization to produce + // ObjectDisposedException, surfacing as MSB4166 "Child node exited prematurely" + // (dotnet/runtime#92290). Eager materialization removes the entire class of issues + // by returning a regular array. Same reasoning as the + // Microsoft.Build.Execution.ProjectItemInstance.TaskItem.EnumerateMetadata twin. + return EnumerateMetadataEager(); } void IMetadataContainer.ImportMetadata(IEnumerable> metadata) @@ -565,7 +562,14 @@ void IMetadataContainer.ImportMetadata(IEnumerable> _metadata = _metadata.SetItems(metadata.Select(kvp => new KeyValuePair(kvp.Key, kvp.Value ?? string.Empty))); } -#if FEATURE_APPDOMAIN + /// + /// Materialize the metadata snapshot eagerly into a heap-allocated array. + /// + /// + /// Used historically only on the AppDomain-crossing path because a yield-iterator state + /// machine is not [Serializable]. Now used unconditionally — see the + /// IMetadataContainer.EnumerateMetadata implementation above for the rationale. + /// private IEnumerable> EnumerateMetadataEager() { if (_metadata == null) @@ -584,20 +588,5 @@ private IEnumerable> EnumerateMetadataEager() return result; } -#endif - - private IEnumerable> EnumerateMetadataLazy() - { - if (_metadata == null) - { - yield break; - } - - foreach (var kvp in _metadata) - { - var unescaped = new KeyValuePair(kvp.Key, EscapingUtilities.UnescapeAll(kvp.Value)); - yield return unescaped; - } - } } }