From dd7ad0e3b3fb6b90b9f5f90278a9fdfddb244e6e Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Sat, 23 Mar 2024 23:29:57 +0100 Subject: [PATCH 1/8] STJ: Dispose enumerator on exception --- .../Collection/IEnumerableDefaultConverter.cs | 60 +++++++++++-------- .../CollectionTests.Generic.Write.cs | 14 +++++ 2 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs index 9a9c62db5708f..e9302eaeb688a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs @@ -18,40 +18,48 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, { Debug.Assert(value is not null); - IEnumerator enumerator; - if (state.Current.CollectionEnumerator == null) + IEnumerator? enumerator = default; + try { - enumerator = value.GetEnumerator(); - if (!enumerator.MoveNext()) + if (state.Current.CollectionEnumerator == null) { - enumerator.Dispose(); - return true; + enumerator = value.GetEnumerator(); + if (!enumerator.MoveNext()) + { + enumerator.Dispose(); + return true; + } } - } - else - { - Debug.Assert(state.Current.CollectionEnumerator is IEnumerator); - enumerator = (IEnumerator)state.Current.CollectionEnumerator; - } - - JsonConverter converter = GetElementConverter(ref state); - do - { - if (ShouldFlush(writer, ref state)) + else { - state.Current.CollectionEnumerator = enumerator; - return false; + Debug.Assert(state.Current.CollectionEnumerator is IEnumerator); + enumerator = (IEnumerator)state.Current.CollectionEnumerator; } - TElement element = enumerator.Current; - if (!converter.TryWrite(writer, element, options, ref state)) + JsonConverter converter = GetElementConverter(ref state); + do { - state.Current.CollectionEnumerator = enumerator; - return false; - } + if (ShouldFlush(writer, ref state)) + { + state.Current.CollectionEnumerator = enumerator; + return false; + } + + TElement element = enumerator.Current; + if (!converter.TryWrite(writer, element, options, ref state)) + { + state.Current.CollectionEnumerator = enumerator; + return false; + } - state.Current.EndCollectionElement(); - } while (enumerator.MoveNext()); + state.Current.EndCollectionElement(); + } while (enumerator.MoveNext()); + } + catch + { + enumerator?.Dispose(); + throw; + } enumerator.Dispose(); return true; diff --git a/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs b/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs index a6131347de3cd..1bde37a79dc12 100644 --- a/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs +++ b/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs @@ -955,6 +955,20 @@ public async Task WriteISetT_DisposesEnumerators() } } + [Fact] + public async Task WriteIEnumerableT_ElementSerializationThrows_DisposesEnumerators() + { + var items = new RefCountedList>(Enumerable.Repeat(ThrowingEnumerable(), 1)); + await Assert.ThrowsAsync(() => Serializer.SerializeWrapper(items.AsEnumerable())); + Assert.Equal(0, items.RefCount); + + static IEnumerable ThrowingEnumerable() + { + yield return 42; + throw new DivideByZeroException(); + } + } + public class SimpleClassWithKeyValuePairs { public KeyValuePair KvpWStrVal { get; set; } From dfbe965ad108eb9ae778b3f166cdbee1dfdb2335 Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Sun, 24 Mar 2024 18:48:24 +0100 Subject: [PATCH 2/8] Avoid code duplication --- .../Collection/IEnumerableDefaultConverter.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs index e9302eaeb688a..ceaefcfd6d51f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs @@ -26,7 +26,6 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, enumerator = value.GetEnumerator(); if (!enumerator.MoveNext()) { - enumerator.Dispose(); return true; } } @@ -42,6 +41,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, if (ShouldFlush(writer, ref state)) { state.Current.CollectionEnumerator = enumerator; + enumerator = default; return false; } @@ -49,20 +49,19 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, if (!converter.TryWrite(writer, element, options, ref state)) { state.Current.CollectionEnumerator = enumerator; + enumerator = default; return false; } state.Current.EndCollectionElement(); } while (enumerator.MoveNext()); + + return true; } - catch + finally { enumerator?.Dispose(); - throw; } - - enumerator.Dispose(); - return true; } } } From 06a96c430fa405d30dcef76f3a4ca8cd924e096e Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Wed, 27 Mar 2024 22:22:48 +0100 Subject: [PATCH 3/8] Rework fix --- .../Collection/DictionaryDefaultConverter.cs | 1 + .../DictionaryOfTKeyTValueConverter.cs | 1 + .../Collection/IEnumerableDefaultConverter.cs | 64 +++++++++---------- .../Metadata/JsonTypeInfoOfT.WriteHelpers.cs | 38 +++++++---- .../Text/Json/Serialization/WriteStack.cs | 28 +++++--- .../Json/Serialization/WriteStackFrame.cs | 5 ++ .../CollectionTests.Generic.Write.cs | 14 ++++ .../Serialization/CollectionTests.cs | 2 + 8 files changed, 97 insertions(+), 56 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs index da360f6140666..3069c458800f9 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs @@ -28,6 +28,7 @@ protected internal override bool OnWriteResume( if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); + state.Current.Disposable = enumerator; if (!enumerator.MoveNext()) { enumerator.Dispose(); 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 dc3f9f5daf306..3d4fddaa8f123 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 @@ -32,6 +32,7 @@ protected internal override bool OnWriteResume( if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); + state.Current.Disposable = enumerator; if (!enumerator.MoveNext()) { enumerator.Dispose(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs index ceaefcfd6d51f..2d2a189d5b389 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs @@ -18,50 +18,44 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, { Debug.Assert(value is not null); - IEnumerator? enumerator = default; - try + IEnumerator enumerator; + if (state.Current.CollectionEnumerator == null) { - if (state.Current.CollectionEnumerator == null) + enumerator = value.GetEnumerator(); + state.Current.Disposable = enumerator; + if (!enumerator.MoveNext()) { - enumerator = value.GetEnumerator(); - if (!enumerator.MoveNext()) - { - return true; - } + enumerator.Dispose(); + return true; } - else + } + else + { + Debug.Assert(state.Current.CollectionEnumerator is IEnumerator); + enumerator = (IEnumerator)state.Current.CollectionEnumerator; + } + + JsonConverter converter = GetElementConverter(ref state); + do + { + if (ShouldFlush(writer, ref state)) { - Debug.Assert(state.Current.CollectionEnumerator is IEnumerator); - enumerator = (IEnumerator)state.Current.CollectionEnumerator; + state.Current.CollectionEnumerator = enumerator; + return false; } - JsonConverter converter = GetElementConverter(ref state); - do + TElement element = enumerator.Current; + if (!converter.TryWrite(writer, element, options, ref state)) { - if (ShouldFlush(writer, ref state)) - { - state.Current.CollectionEnumerator = enumerator; - enumerator = default; - return false; - } - - TElement element = enumerator.Current; - if (!converter.TryWrite(writer, element, options, ref state)) - { - state.Current.CollectionEnumerator = enumerator; - enumerator = default; - return false; - } + state.Current.CollectionEnumerator = enumerator; + return false; + } - state.Current.EndCollectionElement(); - } while (enumerator.MoveNext()); + state.Current.EndCollectionElement(); + } while (enumerator.MoveNext()); - return true; - } - finally - { - enumerator?.Dispose(); - } + enumerator.Dispose(); + return true; } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs index d916334c1925a..6d0cd2c167a94 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs @@ -52,9 +52,17 @@ rootValue is not null && WriteStack state = default; state.Initialize(this, rootValueBoxed); - bool success = EffectiveConverter.WriteCore(writer, rootValue, Options, ref state); - Debug.Assert(success); - writer.Flush(); + try + { + bool success = EffectiveConverter.WriteCore(writer, rootValue, Options, ref state); + Debug.Assert(success); + writer.Flush(); + } + catch + { + state.DisposePendingDisposablesOnException(); + throw; + } } } @@ -244,18 +252,26 @@ rootValue is not null && using var bufferWriter = new PooledByteBufferWriter(Options.DefaultBufferSize); using var writer = new Utf8JsonWriter(bufferWriter, Options.GetWriterOptions()); - do + try { - state.FlushThreshold = (int)(bufferWriter.Capacity * JsonSerializer.FlushThreshold); + do + { + state.FlushThreshold = (int)(bufferWriter.Capacity * JsonSerializer.FlushThreshold); - isFinalBlock = EffectiveConverter.WriteCore(writer, rootValue, Options, ref state); - writer.Flush(); + isFinalBlock = EffectiveConverter.WriteCore(writer, rootValue, Options, ref state); + writer.Flush(); - bufferWriter.WriteToStream(utf8Json); - bufferWriter.Clear(); + bufferWriter.WriteToStream(utf8Json); + bufferWriter.Clear(); - Debug.Assert(state.PendingTask == null); - } while (!isFinalBlock); + Debug.Assert(state.PendingTask == null); + } while (!isFinalBlock); + } + catch + { + state.DisposePendingDisposablesOnException(); + throw; + } if (CanUseSerializeHandler) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs index 32ab005991b5d..e9ede247cdacc 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs @@ -309,13 +309,13 @@ public void DisposePendingDisposablesOnException() Exception? exception = null; Debug.Assert(Current.AsyncDisposable is null); - DisposeFrame(Current.CollectionEnumerator, ref exception); + DisposeFrame(Current.CollectionEnumerator, Current.Disposable, ref exception); int stackSize = Math.Max(_count, _continuationCount); for (int i = 0; i < stackSize - 1; i++) { Debug.Assert(_stack[i].AsyncDisposable is null); - DisposeFrame(_stack[i].CollectionEnumerator, ref exception); + DisposeFrame(_stack[i].CollectionEnumerator, _stack[i].Disposable, ref exception); } if (exception is not null) @@ -323,13 +323,17 @@ public void DisposePendingDisposablesOnException() ExceptionDispatchInfo.Capture(exception).Throw(); } - static void DisposeFrame(IEnumerator? collectionEnumerator, ref Exception? exception) + static void DisposeFrame(IEnumerator? collectionEnumerator, IDisposable? disposable, ref Exception? exception) { try { - if (collectionEnumerator is IDisposable disposable) + if (collectionEnumerator is IDisposable disposableEnumerator) { - disposable.Dispose(); + disposableEnumerator.Dispose(); + } + else + { + disposable?.Dispose(); } } catch (Exception e) @@ -347,12 +351,12 @@ public async ValueTask DisposePendingDisposablesOnExceptionAsync() { Exception? exception = null; - exception = await DisposeFrame(Current.CollectionEnumerator, Current.AsyncDisposable, exception).ConfigureAwait(false); + exception = await DisposeFrame(Current.CollectionEnumerator, Current.AsyncDisposable, Current.Disposable, exception).ConfigureAwait(false); int stackSize = Math.Max(_count, _continuationCount); for (int i = 0; i < stackSize - 1; i++) { - exception = await DisposeFrame(_stack[i].CollectionEnumerator, _stack[i].AsyncDisposable, exception).ConfigureAwait(false); + exception = await DisposeFrame(_stack[i].CollectionEnumerator, _stack[i].AsyncDisposable, _stack[i].Disposable, exception).ConfigureAwait(false); } if (exception is not null) @@ -360,20 +364,24 @@ public async ValueTask DisposePendingDisposablesOnExceptionAsync() ExceptionDispatchInfo.Capture(exception).Throw(); } - static async ValueTask DisposeFrame(IEnumerator? collectionEnumerator, IAsyncDisposable? asyncDisposable, Exception? exception) + static async ValueTask DisposeFrame(IEnumerator? collectionEnumerator, IAsyncDisposable? asyncDisposable, IDisposable? disposable, Exception? exception) { Debug.Assert(!(collectionEnumerator is not null && asyncDisposable is not null)); try { - if (collectionEnumerator is IDisposable disposable) + if (collectionEnumerator is IDisposable disposableEnumerator) { - disposable.Dispose(); + disposableEnumerator.Dispose(); } else if (asyncDisposable is not null) { await asyncDisposable.DisposeAsync().ConfigureAwait(false); } + else + { + disposable?.Dispose(); + } } catch (Exception e) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs index a2b6a787c6ac4..eb582ec08c966 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs @@ -18,6 +18,11 @@ internal struct WriteStackFrame /// public IEnumerator? CollectionEnumerator; + /// + /// The enumerator for resumable disposables. + /// + public IDisposable? Disposable; + /// /// The enumerator for resumable async disposables. /// diff --git a/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs b/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs index 1bde37a79dc12..19afcd962c750 100644 --- a/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs +++ b/src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Generic.Write.cs @@ -969,6 +969,20 @@ static IEnumerable ThrowingEnumerable() } } + [Fact] + public async Task WriteIDictionaryT_ElementSerializationThrows_DisposesEnumerators() + { + var items = new RefCountedDictionary>(Enumerable.Repeat(new KeyValuePair>(42, ThrowingEnumerable()), 1)); + await Assert.ThrowsAsync(() => Serializer.SerializeWrapper((IDictionary>)items)); + Assert.Equal(0, items.RefCount); + + static IEnumerable ThrowingEnumerable() + { + yield return 42; + throw new DivideByZeroException(); + } + } + public class SimpleClassWithKeyValuePairs { public KeyValuePair KvpWStrVal { get; set; } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/CollectionTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/CollectionTests.cs index 7463dfbfbb6bc..79528a4a61749 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/CollectionTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/CollectionTests.cs @@ -130,6 +130,7 @@ public async Task DeserializeAsyncEnumerable() [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(string))] [JsonSerializable(typeof(IDictionary))] + [JsonSerializable(typeof(IDictionary>))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary>))] @@ -556,6 +557,7 @@ public CollectionTests_Default() [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(string))] [JsonSerializable(typeof(IDictionary))] + [JsonSerializable(typeof(IDictionary>))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary))] [JsonSerializable(typeof(Dictionary>))] From 1ae7d96c6eaa7506b498866e05190c189821e93e Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Thu, 28 Mar 2024 22:17:23 +0100 Subject: [PATCH 4/8] Remove useless Disposable field --- .../Collection/DictionaryDefaultConverter.cs | 2 +- .../DictionaryOfTKeyTValueConverter.cs | 2 +- .../Collection/IEnumerableDefaultConverter.cs | 2 +- .../Text/Json/Serialization/WriteStack.cs | 28 +++++++------------ .../Json/Serialization/WriteStackFrame.cs | 5 ---- 5 files changed, 13 insertions(+), 26 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs index 3069c458800f9..73b7124e97af1 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs @@ -28,7 +28,7 @@ protected internal override bool OnWriteResume( if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); - state.Current.Disposable = enumerator; + state.Current.CollectionEnumerator = enumerator; if (!enumerator.MoveNext()) { enumerator.Dispose(); 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 3d4fddaa8f123..d3f804a4a0f16 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 @@ -32,7 +32,7 @@ protected internal override bool OnWriteResume( if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); - state.Current.Disposable = enumerator; + state.Current.CollectionEnumerator = enumerator; if (!enumerator.MoveNext()) { enumerator.Dispose(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs index 2d2a189d5b389..19002355cc25d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs @@ -22,7 +22,7 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); - state.Current.Disposable = enumerator; + state.Current.CollectionEnumerator = enumerator; if (!enumerator.MoveNext()) { enumerator.Dispose(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs index e9ede247cdacc..32ab005991b5d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs @@ -309,13 +309,13 @@ public void DisposePendingDisposablesOnException() Exception? exception = null; Debug.Assert(Current.AsyncDisposable is null); - DisposeFrame(Current.CollectionEnumerator, Current.Disposable, ref exception); + DisposeFrame(Current.CollectionEnumerator, ref exception); int stackSize = Math.Max(_count, _continuationCount); for (int i = 0; i < stackSize - 1; i++) { Debug.Assert(_stack[i].AsyncDisposable is null); - DisposeFrame(_stack[i].CollectionEnumerator, _stack[i].Disposable, ref exception); + DisposeFrame(_stack[i].CollectionEnumerator, ref exception); } if (exception is not null) @@ -323,17 +323,13 @@ public void DisposePendingDisposablesOnException() ExceptionDispatchInfo.Capture(exception).Throw(); } - static void DisposeFrame(IEnumerator? collectionEnumerator, IDisposable? disposable, ref Exception? exception) + static void DisposeFrame(IEnumerator? collectionEnumerator, ref Exception? exception) { try { - if (collectionEnumerator is IDisposable disposableEnumerator) + if (collectionEnumerator is IDisposable disposable) { - disposableEnumerator.Dispose(); - } - else - { - disposable?.Dispose(); + disposable.Dispose(); } } catch (Exception e) @@ -351,12 +347,12 @@ public async ValueTask DisposePendingDisposablesOnExceptionAsync() { Exception? exception = null; - exception = await DisposeFrame(Current.CollectionEnumerator, Current.AsyncDisposable, Current.Disposable, exception).ConfigureAwait(false); + exception = await DisposeFrame(Current.CollectionEnumerator, Current.AsyncDisposable, exception).ConfigureAwait(false); int stackSize = Math.Max(_count, _continuationCount); for (int i = 0; i < stackSize - 1; i++) { - exception = await DisposeFrame(_stack[i].CollectionEnumerator, _stack[i].AsyncDisposable, _stack[i].Disposable, exception).ConfigureAwait(false); + exception = await DisposeFrame(_stack[i].CollectionEnumerator, _stack[i].AsyncDisposable, exception).ConfigureAwait(false); } if (exception is not null) @@ -364,24 +360,20 @@ public async ValueTask DisposePendingDisposablesOnExceptionAsync() ExceptionDispatchInfo.Capture(exception).Throw(); } - static async ValueTask DisposeFrame(IEnumerator? collectionEnumerator, IAsyncDisposable? asyncDisposable, IDisposable? disposable, Exception? exception) + static async ValueTask DisposeFrame(IEnumerator? collectionEnumerator, IAsyncDisposable? asyncDisposable, Exception? exception) { Debug.Assert(!(collectionEnumerator is not null && asyncDisposable is not null)); try { - if (collectionEnumerator is IDisposable disposableEnumerator) + if (collectionEnumerator is IDisposable disposable) { - disposableEnumerator.Dispose(); + disposable.Dispose(); } else if (asyncDisposable is not null) { await asyncDisposable.DisposeAsync().ConfigureAwait(false); } - else - { - disposable?.Dispose(); - } } catch (Exception e) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs index eb582ec08c966..a2b6a787c6ac4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs @@ -18,11 +18,6 @@ internal struct WriteStackFrame /// public IEnumerator? CollectionEnumerator; - /// - /// The enumerator for resumable disposables. - /// - public IDisposable? Disposable; - /// /// The enumerator for resumable async disposables. /// From 7734059911c087b1448dc1713cca0732d323874d Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Sat, 30 Mar 2024 20:27:11 +0100 Subject: [PATCH 5/8] Apply fix on all collection converters --- .../Serialization/Converters/Collection/IDictionaryConverter.cs | 1 + .../Serialization/Converters/Collection/IEnumerableConverter.cs | 1 + .../Serialization/Converters/Collection/StackOrQueueConverter.cs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs index 9f8a48140c6be..74c150e0f9014 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs @@ -45,6 +45,7 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TDictionar if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); + state.Current.CollectionEnumerator = enumerator; if (!enumerator.MoveNext()) { return true; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs index 767bae403cf34..f7fd18f1c4b59 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs @@ -46,6 +46,7 @@ protected override bool OnWriteResume( if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); + state.Current.CollectionEnumerator = enumerator; if (!enumerator.MoveNext()) { return true; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs index 7831bb5fdc873..8540d7ea1aa5e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs @@ -46,6 +46,7 @@ protected sealed override bool OnWriteResume(Utf8JsonWriter writer, TCollection if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); + state.Current.CollectionEnumerator = enumerator; if (!enumerator.MoveNext()) { return true; From 576c426f433ee6fee20fdbb86388a8fd1b92de64 Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Sat, 30 Mar 2024 21:15:59 +0100 Subject: [PATCH 6/8] Remove duplicate assignments --- .../Converters/Collection/DictionaryDefaultConverter.cs | 2 -- .../Collection/DictionaryOfTKeyTValueConverter.cs | 6 ++---- .../Converters/Collection/IDictionaryConverter.cs | 2 -- .../Converters/Collection/IEnumerableConverter.cs | 2 -- .../Converters/Collection/IEnumerableDefaultConverter.cs | 2 -- .../Converters/Collection/StackOrQueueConverter.cs | 2 -- 6 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs index 73b7124e97af1..05929fbe9fd20 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs @@ -48,7 +48,6 @@ protected internal override bool OnWriteResume( { if (ShouldFlush(writer, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } @@ -62,7 +61,6 @@ protected internal override bool OnWriteResume( TValue element = enumerator.Current.Value; if (!_valueConverter.TryWrite(writer, element, options, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } 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 d3f804a4a0f16..d232f73ee3a9f 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 @@ -28,7 +28,7 @@ protected internal override bool OnWriteResume( JsonSerializerOptions options, ref WriteStack state) { - Dictionary.Enumerator enumerator; + IEnumerator> enumerator; if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); @@ -41,7 +41,7 @@ protected internal override bool OnWriteResume( } else { - enumerator = (Dictionary.Enumerator)state.Current.CollectionEnumerator; + enumerator = (IEnumerator>)state.Current.CollectionEnumerator; } JsonTypeInfo typeInfo = state.Current.JsonTypeInfo; @@ -64,7 +64,6 @@ protected internal override bool OnWriteResume( { if (ShouldFlush(writer, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } @@ -79,7 +78,6 @@ protected internal override bool OnWriteResume( TValue element = enumerator.Current.Value; if (!_valueConverter.TryWrite(writer, element, options, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs index 74c150e0f9014..d109b254659f0 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs @@ -63,7 +63,6 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TDictionar { if (ShouldFlush(writer, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } @@ -88,7 +87,6 @@ protected internal override bool OnWriteResume(Utf8JsonWriter writer, TDictionar object? element = enumerator.Value; if (!_valueConverter.TryWrite(writer, element, options, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs index f7fd18f1c4b59..7fa9130e6557b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverter.cs @@ -62,14 +62,12 @@ protected override bool OnWriteResume( { if (ShouldFlush(writer, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } object? element = enumerator.Current; if (!converter.TryWrite(writer, element, options, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs index 19002355cc25d..b8b7a4661e51e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs @@ -40,14 +40,12 @@ protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, { if (ShouldFlush(writer, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } TElement element = enumerator.Current; if (!converter.TryWrite(writer, element, options, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs index 8540d7ea1aa5e..cfa54f31882e0 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/StackOrQueueConverter.cs @@ -62,14 +62,12 @@ protected sealed override bool OnWriteResume(Utf8JsonWriter writer, TCollection { if (ShouldFlush(writer, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } object? element = enumerator.Current; if (!converter.TryWrite(writer, element, options, ref state)) { - state.Current.CollectionEnumerator = enumerator; return false; } From a2861f397ebbc68047692fed9032f36b414ef84d Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Wed, 3 Apr 2024 00:18:00 +0200 Subject: [PATCH 7/8] Skip fix for no-op Dispose implementation --- .../Collection/DictionaryOfTKeyTValueConverter.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 d232f73ee3a9f..dc3f9f5daf306 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 @@ -28,11 +28,10 @@ protected internal override bool OnWriteResume( JsonSerializerOptions options, ref WriteStack state) { - IEnumerator> enumerator; + Dictionary.Enumerator enumerator; if (state.Current.CollectionEnumerator == null) { enumerator = value.GetEnumerator(); - state.Current.CollectionEnumerator = enumerator; if (!enumerator.MoveNext()) { enumerator.Dispose(); @@ -41,7 +40,7 @@ protected internal override bool OnWriteResume( } else { - enumerator = (IEnumerator>)state.Current.CollectionEnumerator; + enumerator = (Dictionary.Enumerator)state.Current.CollectionEnumerator; } JsonTypeInfo typeInfo = state.Current.JsonTypeInfo; @@ -64,6 +63,7 @@ protected internal override bool OnWriteResume( { if (ShouldFlush(writer, ref state)) { + state.Current.CollectionEnumerator = enumerator; return false; } @@ -78,6 +78,7 @@ protected internal override bool OnWriteResume( TValue element = enumerator.Current.Value; if (!_valueConverter.TryWrite(writer, element, options, ref state)) { + state.Current.CollectionEnumerator = enumerator; return false; } From 1b36403240f6bbcf00659757b25bc71c736cd432 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 24 Jun 2024 17:05:17 +0100 Subject: [PATCH 8/8] Move IEnumerator disposal to WriteCore method. --- .../JsonConverterOfT.WriteCore.cs | 47 ++++++++++--------- .../Metadata/JsonTypeInfoOfT.WriteHelpers.cs | 15 ++---- .../Text/Json/Serialization/WriteStack.cs | 6 +-- .../Text/Json/ThrowHelper.Serialization.cs | 6 +-- 4 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs index 8f1d8be16f89e..1a7636bdc5252 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs @@ -15,32 +15,37 @@ internal bool WriteCore( { return TryWrite(writer, value, options, ref state); } - catch (InvalidOperationException ex) when (ex.Source == ThrowHelper.ExceptionSourceValueToRethrowAsJsonException) + catch (Exception ex) { - ThrowHelper.ReThrowWithPath(ref state, ex); - throw; - } - catch (JsonException ex) when (ex.Path == null) - { - // JsonExceptions where the Path property is already set - // typically originate from nested calls to JsonSerializer; - // treat these cases as any other exception type and do not - // overwrite any exception information. + if (!state.SupportAsync) + { + // Async serializers should dispose sync and + // async disposables from the async root method. + state.DisposePendingDisposablesOnException(); + } - ThrowHelper.AddJsonExceptionInformation(ref state, ex); - throw; - } - catch (NotSupportedException ex) - { - // If the message already contains Path, just re-throw. This could occur in serializer re-entry cases. - // To get proper Path semantics in re-entry cases, APIs that take 'state' need to be used. - if (ex.Message.Contains(" Path: ")) + switch (ex) { - throw; + case InvalidOperationException when ex.Source == ThrowHelper.ExceptionSourceValueToRethrowAsJsonException: + ThrowHelper.ReThrowWithPath(ref state, ex); + break; + + case JsonException { Path: null } jsonException: + // JsonExceptions where the Path property is already set + // typically originate from nested calls to JsonSerializer; + // treat these cases as any other exception type and do not + // overwrite any exception information. + ThrowHelper.AddJsonExceptionInformation(ref state, jsonException); + break; + + case NotSupportedException when !ex.Message.Contains(" Path: "): + // If the message already contains Path, just re-throw. This could occur in serializer re-entry cases. + // To get proper Path semantics in re-entry cases, APIs that take 'state' need to be used. + ThrowHelper.ThrowNotSupportedException(ref state, ex); + break; } - ThrowHelper.ThrowNotSupportedException(ref state, ex); - return default; + throw; } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs index bf7e92c2e9066..c14eeb7ef78ed 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs @@ -53,17 +53,9 @@ rootValue is not null && WriteStack state = default; state.Initialize(this, rootValueBoxed); - try - { - bool success = EffectiveConverter.WriteCore(writer, rootValue, Options, ref state); - Debug.Assert(success); - writer.Flush(); - } - catch - { - state.DisposePendingDisposablesOnException(); - throw; - } + bool success = EffectiveConverter.WriteCore(writer, rootValue, Options, ref state); + Debug.Assert(success); + writer.Flush(); } } @@ -313,6 +305,7 @@ rootValue is not null && { ThrowHelper.ThrowInvalidOperationException_PipeWriterDoesNotImplementUnflushedBytes(bufferWriter); } + state.PipeWriter = bufferWriter; state.FlushThreshold = (int)(bufferWriter.Capacity * JsonSerializer.FlushThreshold); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs index 38517ffab7f01..30151d67ad8c7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs @@ -278,7 +278,7 @@ public void AddCompletedAsyncDisposable(IAsyncDisposable asyncDisposable) => (CompletedAsyncDisposables ??= new List()).Add(asyncDisposable); // Asynchronously dispose of any AsyncDisposables that have been scheduled for disposal - public async ValueTask DisposeCompletedAsyncDisposables() + public readonly async ValueTask DisposeCompletedAsyncDisposables() { Debug.Assert(CompletedAsyncDisposables?.Count > 0); Exception? exception = null; @@ -307,7 +307,7 @@ public async ValueTask DisposeCompletedAsyncDisposables() /// Walks the stack cleaning up any leftover IDisposables /// in the event of an exception on serialization /// - public void DisposePendingDisposablesOnException() + public readonly void DisposePendingDisposablesOnException() { Exception? exception = null; @@ -346,7 +346,7 @@ static void DisposeFrame(IEnumerator? collectionEnumerator, ref Exception? excep /// Walks the stack cleaning up any leftover I(Async)Disposables /// in the event of an exception on async serialization /// - public async ValueTask DisposePendingDisposablesOnExceptionAsync() + public readonly async ValueTask DisposePendingDisposablesOnExceptionAsync() { Exception? exception = null; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs index 4a2759a57c398..226e60c80c282 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs @@ -586,9 +586,9 @@ public static void ThrowNotSupportedException(scoped ref ReadStack state, in Utf } [DoesNotReturn] - public static void ThrowNotSupportedException(ref WriteStack state, NotSupportedException ex) + public static void ThrowNotSupportedException(ref WriteStack state, Exception innerException) { - string message = ex.Message; + string message = innerException.Message; // The caller should check to ensure path is not already set. Debug.Assert(!message.Contains(" Path: ")); @@ -608,7 +608,7 @@ public static void ThrowNotSupportedException(ref WriteStack state, NotSupported message += $" Path: {state.PropertyPath()}."; - throw new NotSupportedException(message, ex); + throw new NotSupportedException(message, innerException); } [DoesNotReturn]