Fix undisposed enumerators (#48322)

-Fix converters not disposing enumerators (issue #46349) -Add tests
dotnet · Feb 16, 2021 · 8f91099 · 8f91099
1 parent 7ec683c
commit 8f91099
Show file tree

Hide file tree

Showing 15 changed files with 176 additions and 0 deletions.
diff --git a/...n/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentQueueOfTConverter.cs b/...n/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/...n/src/System/Text/Json/Serialization/Converters/Collection/ConcurrentStackOfTConverter.cs b/...n/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/...c/System/Text/Json/Serialization/Converters/Collection/DictionaryOfTKeyTValueConverter.cs b/...c/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/....Json/src/System/Text/Json/Serialization/Converters/Collection/ICollectionOfTConverter.cs b/....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/.../System/Text/Json/Serialization/Converters/Collection/IDictionaryOfTKeyTValueConverter.cs b/.../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/....Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableOfTConverter.cs b/....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/...m.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IListOfTConverter.cs b/...m.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/...Text/Json/Serialization/Converters/Collection/IReadOnlyDictionaryOfTKeyTValueConverter.cs b/...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/...em.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ISetOfTConverter.cs b/...em.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/...Text/Json/Serialization/Converters/Collection/ImmutableDictionaryOfTKeyTValueConverter.cs b/...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/...c/System/Text/Json/Serialization/Converters/Collection/ImmutableEnumerableOfTConverter.cs b/...c/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/...m.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/QueueOfTConverter.cs b/...m.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/...m.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOfTConverter.cs b/...m.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/...ies/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Write.cs b/...ies/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Write.cs
@@ -882,6 +882,78 @@ public static void ConvertIEnumerableValueTypesThenSerialize()
             Assert.Equal(expectedJson, JsonSerializer.Serialize<IEnumerable<ValueB>>(valueBs));
         }
 
+        [Fact]
+        public static void WriteIEnumerableT_DisposesEnumerators()
+        {
+            for (int count = 0; count < 5; count++)
+            {
+                var items = new RefCountedList<int>(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<int>(Enumerable.Range(1, count));
+                _ = JsonSerializer.Serialize((ICollection<int>)items);
+
+                Assert.Equal(0, items.RefCount);
+            }
+        }
+
+        [Fact]
+        public static void WriteIListT_DisposesEnumerators()
+        {
+            for (int count = 0; count < 5; count++)
+            {
+                var items = new RefCountedList<int>(Enumerable.Range(1, count));
+                _ = JsonSerializer.Serialize((IList<int>)items);
+
+                Assert.Equal(0, items.RefCount);
+            }
+        }
+
+        [Fact]
+        public static void WriteIDictionaryT_DisposesEnumerators()
+        {
+            for (int count = 0; count < 5; count++)
+            {
+                var pairs = new RefCountedDictionary<int, int>(Enumerable.Range(1, count).Select(x => new KeyValuePair<int, int>(x, x)));
+                _ = JsonSerializer.Serialize((IDictionary<int, int>)pairs);
+
+                Assert.Equal(0, pairs.RefCount);
+            }
+        }
+
+        [Fact]
+        public static void WriteIReadOnlyDictionaryT_DisposesEnumerators()
+        {
+            for (int count = 0; count < 5; count++)
+            {
+                var pairs = new RefCountedDictionary<int, int>(Enumerable.Range(1, count).Select(x => new KeyValuePair<int, int>(x, x)));
+                _ = JsonSerializer.Serialize((IReadOnlyDictionary<int, int>)pairs);
+
+                Assert.Equal(0, pairs.RefCount);
+            }
+        }
+
+        [Fact]
+        public static void WriteISetT_DisposesEnumerators()
+        {
+            for (int count = 0; count < 5; count++)
+            {
+                var items = new RefCountedSet<int>(Enumerable.Range(1, count));
+                _ = JsonSerializer.Serialize((ISet<int>)items);
+
+                Assert.Equal(0, items.RefCount);
+            }
+        }
+
         public class SimpleClassWithKeyValuePairs
         {
             public KeyValuePair<string, string> KvpWStrVal { get; set; }
@@ -901,5 +973,57 @@ public class ValueB
         {
             public int Value { get; set; }
         }
+
+        private class RefCountedList<T> : List<T>, IEnumerable<T>   //  Reimplement interface.
+        {
+            public RefCountedList() : base() { }
+            public RefCountedList(IEnumerable<T> collection) : base(collection) { }
+
+            public int RefCount { get; private set; }
+
+            IEnumerator<T> IEnumerable<T>.GetEnumerator()
+            {
+                RefCount++;
+                return new DisposableEnumerator<T>(GetEnumerator(), () => RefCount--);
+            }
+
+            Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => this.AsEnumerable().GetEnumerator();
+        }
+
+        private class RefCountedDictionary<TKey, TValue> : Dictionary<TKey, TValue>, IEnumerable<KeyValuePair<TKey, TValue>>    //  Reimplement interface.
+        {
+            public RefCountedDictionary() : base() { }
+            public RefCountedDictionary(IEnumerable<KeyValuePair<TKey, TValue>> collection)
+            {
+                foreach (var kvp in collection)
+                    Add(kvp.Key, kvp.Value);
+            }
+
+            public int RefCount { get; private set; }
+
+            IEnumerator<KeyValuePair<TKey, TValue>> IEnumerable<KeyValuePair<TKey, TValue>>.GetEnumerator()
+            {
+                RefCount++;
+                return new DisposableEnumerator<KeyValuePair<TKey, TValue>>(GetEnumerator(), () => RefCount--);
+            }
+
+            Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => this.AsEnumerable().GetEnumerator();
+        }
+
+        private class RefCountedSet<T> : HashSet<T>, IEnumerable<T>     // Reimplement interface.
+        {
+            public RefCountedSet() : base() { }
+            public RefCountedSet(IEnumerable<T> collection) : base(collection) { }
+
+            public int RefCount { get; private set; }
+
+            IEnumerator<T> IEnumerable<T>.GetEnumerator()
+            {
+                RefCount++;
+                return new DisposableEnumerator<T>(GetEnumerator(), () => RefCount--);
+            }
+
+            Collections.IEnumerator Collections.IEnumerable.GetEnumerator() => this.AsEnumerable().GetEnumerator();
+        }
     }
 }
diff --git a/...raries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.GenericCollections.cs b/...raries/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<T> : IEnumerator<T>
+    {
+        private readonly IEnumerator<T> _inner;
+        private Action _onDispose;
+
+        public DisposableEnumerator(IEnumerator<T> 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();
+    }
 }