From 8f91099069cfbad7ed53ab39dae59132bc3feb41 Mon Sep 17 00:00:00 2001 From: Newell Clark Date: Tue, 16 Feb 2021 13:33:44 -0500 Subject: [PATCH] Fix undisposed enumerators (#48322) -Fix converters not disposing enumerators (issue #46349) -Add tests --- .../Collection/ConcurrentQueueOfTConverter.cs | 2 + .../Collection/ConcurrentStackOfTConverter.cs | 2 + .../DictionaryOfTKeyTValueConverter.cs | 2 + .../Collection/ICollectionOfTConverter.cs | 2 + .../IDictionaryOfTKeyTValueConverter.cs | 2 + .../Collection/IEnumerableOfTConverter.cs | 2 + .../Collection/IListOfTConverter.cs | 2 + ...ReadOnlyDictionaryOfTKeyTValueConverter.cs | 2 + .../Converters/Collection/ISetOfTConverter.cs | 2 + ...mmutableDictionaryOfTKeyTValueConverter.cs | 2 + .../ImmutableEnumerableOfTConverter.cs | 2 + .../Collection/QueueOfTConverter.cs | 2 + .../Collection/StackOfTConverter.cs | 2 + .../CollectionTests.Generic.Write.cs | 124 ++++++++++++++++++ .../TestClasses.GenericCollections.cs | 26 ++++ 15 files changed, 176 insertions(+) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentQueueOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentQueueOfTConverter.cs index 67873f7851b7e..afe3b2a02c751 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentQueueOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentQueueOfTConverter.cs @@ -33,6 +33,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -58,6 +59,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentStackOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentStackOfTConverter.cs index ad80300a8a6db..60529f8c95986 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentStackOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentStackOfTConverter.cs @@ -33,6 +33,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -58,6 +59,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryOfTKeyTValueConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryOfTKeyTValueConverter.cs index c89e1f9be07ee..4528ddc8b104d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryOfTKeyTValueConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryOfTKeyTValueConverter.cs @@ -41,6 +41,7 @@ protected internal override bool OnWriteResume( enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -93,6 +94,7 @@ protected internal override bool OnWriteResume( } while (enumerator.MoveNext()); } + enumerator.Dispose(); return true; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ICollectionOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ICollectionOfTConverter.cs index e78add4a98fda..f6fc439f8521d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ICollectionOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ICollectionOfTConverter.cs @@ -65,6 +65,7 @@ protected override bool OnWriteResume( enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -90,6 +91,7 @@ protected override bool OnWriteResume( } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryOfTKeyTValueConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryOfTKeyTValueConverter.cs index da9dcf1040d36..ad2d185203801 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryOfTKeyTValueConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryOfTKeyTValueConverter.cs @@ -67,6 +67,7 @@ protected internal override bool OnWriteResume( enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -102,6 +103,7 @@ protected internal override bool OnWriteResume( state.Current.EndDictionaryElement(); } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableOfTConverter.cs index d4a3738f2f596..25eb1c9e0dd02 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableOfTConverter.cs @@ -36,6 +36,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -62,6 +63,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IListOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IListOfTConverter.cs index 72f004440b3c3..e24788a53e4b7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IListOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IListOfTConverter.cs @@ -61,6 +61,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -86,6 +87,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlyDictionaryOfTKeyTValueConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlyDictionaryOfTKeyTValueConverter.cs index f8156f51c181c..8a9f452adf7a0 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlyDictionaryOfTKeyTValueConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IReadOnlyDictionaryOfTKeyTValueConverter.cs @@ -33,6 +33,7 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TCollectio enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -69,6 +70,7 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TCollectio state.Current.EndDictionaryElement(); } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ISetOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ISetOfTConverter.cs index ea66b0e56f169..7938a08090ce1 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ISetOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ISetOfTConverter.cs @@ -58,6 +58,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -83,6 +84,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableDictionaryOfTKeyTValueConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableDictionaryOfTKeyTValueConverter.cs index f2ade30640030..f9d73d5df3cf0 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableDictionaryOfTKeyTValueConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableDictionaryOfTKeyTValueConverter.cs @@ -44,6 +44,7 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TCollectio enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -80,6 +81,7 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TCollectio state.Current.EndDictionaryElement(); } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableEnumerableOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableEnumerableOfTConverter.cs index eeb262b8ddb7d..488c191ac9b33 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableEnumerableOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ImmutableEnumerableOfTConverter.cs @@ -43,6 +43,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -68,6 +69,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/QueueOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/QueueOfTConverter.cs index 2793933b26c1b..69dade98b40af 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/QueueOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/QueueOfTConverter.cs @@ -32,6 +32,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -57,6 +58,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOfTConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOfTConverter.cs index 42b8bc93824a8..df8e0bd17d635 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOfTConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOfTConverter.cs @@ -32,6 +32,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { + enumerator.Dispose(); return true; } } @@ -57,6 +58,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, } } while (enumerator.MoveNext()); + enumerator.Dispose(); return true; } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Write.cs b/src/libraries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Write.cs index e9f3def1a427d..8c968f918e204 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Write.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Write.cs @@ -882,6 +882,78 @@ public static void ConvertIEnumerableValueTypesThenSerialize() Assert.Equal(expectedJson, JsonSerializer.Serialize>(valueBs)); } + [Fact] + public static void WriteIEnumerableT_DisposesEnumerators() + { + for (int count = 0; count < 5; count++) + { + var items = new RefCountedList(Enumerable.Range(1, count)); + _ = JsonSerializer.Serialize(items.AsEnumerable()); + + Assert.Equal(0, items.RefCount); + } + } + + [Fact] + public static void WriteICollectionT_DisposesEnumerators() + { + for (int count = 0; count < 5; count++) + { + var items = new RefCountedList(Enumerable.Range(1, count)); + _ = JsonSerializer.Serialize((ICollection)items); + + Assert.Equal(0, items.RefCount); + } + } + + [Fact] + public static void WriteIListT_DisposesEnumerators() + { + for (int count = 0; count < 5; count++) + { + var items = new RefCountedList(Enumerable.Range(1, count)); + _ = JsonSerializer.Serialize((IList)items); + + Assert.Equal(0, items.RefCount); + } + } + + [Fact] + public static void WriteIDictionaryT_DisposesEnumerators() + { + for (int count = 0; count < 5; count++) + { + var pairs = new RefCountedDictionary(Enumerable.Range(1, count).Select(x => new KeyValuePair(x, x))); + _ = JsonSerializer.Serialize((IDictionary)pairs); + + Assert.Equal(0, pairs.RefCount); + } + } + + [Fact] + public static void WriteIReadOnlyDictionaryT_DisposesEnumerators() + { + for (int count = 0; count < 5; count++) + { + var pairs = new RefCountedDictionary(Enumerable.Range(1, count).Select(x => new KeyValuePair(x, x))); + _ = JsonSerializer.Serialize((IReadOnlyDictionary)pairs); + + Assert.Equal(0, pairs.RefCount); + } + } + + [Fact] + public static void WriteISetT_DisposesEnumerators() + { + for (int count = 0; count < 5; count++) + { + var items = new RefCountedSet(Enumerable.Range(1, count)); + _ = JsonSerializer.Serialize((ISet)items); + + Assert.Equal(0, items.RefCount); + } + } + public class SimpleClassWithKeyValuePairs { public KeyValuePair KvpWStrVal { get; set; } @@ -901,5 +973,57 @@ public class ValueB { public int Value { get; set; } } + + private class RefCountedList : List, IEnumerable // Reimplement interface. + { + public RefCountedList() : base() { } + public RefCountedList(IEnumerable collection) : base(collection) { } + + public int RefCount { get; private set; } + + IEnumerator IEnumerable.GetEnumerator() + { + RefCount++; + return new DisposableEnumerator(GetEnumerator(), () => RefCount--); + } + + Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => this.AsEnumerable().GetEnumerator(); + } + + private class RefCountedDictionary : Dictionary, IEnumerable> // Reimplement interface. + { + public RefCountedDictionary() : base() { } + public RefCountedDictionary(IEnumerable> collection) + { + foreach (var kvp in collection) + Add(kvp.Key, kvp.Value); + } + + public int RefCount { get; private set; } + + IEnumerator> IEnumerable>.GetEnumerator() + { + RefCount++; + return new DisposableEnumerator>(GetEnumerator(), () => RefCount--); + } + + Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => this.AsEnumerable().GetEnumerator(); + } + + private class RefCountedSet : HashSet, IEnumerable // Reimplement interface. + { + public RefCountedSet() : base() { } + public RefCountedSet(IEnumerable collection) : base(collection) { } + + public int RefCount { get; private set; } + + IEnumerator IEnumerable.GetEnumerator() + { + RefCount++; + return new DisposableEnumerator(GetEnumerator(), () => RefCount--); + } + + Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => this.AsEnumerable().GetEnumerator(); + } } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.GenericCollections.cs b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.GenericCollections.cs index fabecba642996..356f1e782cea7 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.GenericCollections.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.GenericCollections.cs @@ -1641,4 +1641,30 @@ public void Verify() Assert.Equal("value1", Dictionary.Value["key1"]); } } + + public class DisposableEnumerator : IEnumerator + { + private readonly IEnumerator _inner; + private Action _onDispose; + + public DisposableEnumerator(IEnumerator inner, Action onDispose) + { + _inner = inner; + _onDispose = onDispose; + } + + public T Current => _inner.Current; + + object IEnumerator.Current => ((IEnumerator)_inner).Current; + + public bool MoveNext() => _inner.MoveNext(); + + public void Dispose() + { + _onDispose?.Invoke(); + _onDispose = null; + } + + public void Reset() => _inner.Reset(); + } }