Dispose IAsyncEnumerable enumerators inline during property-based JSON serialization

[Copilot]

Description

DisposeAsync() was never invoked for property-based IAsyncEnumerable<T> during async JSON serialization. Enumerators were queued into a CompletedAsyncDisposables list and only disposed after the entire object finished serializing — meaning sibling IAsyncEnumerable<T> properties would start enumeration before prior enumerators were disposed. This caused resource leaks, most visibly with EF Core where PostgreSQL connections remained open.

// Second property's enumeration starts before first's enumerator is disposed
await JsonSerializer.SerializeAsync(stream, new {
    A = dbContext.Products.Where(p => p.Id % 2 == 0).AsAsyncEnumerable(),
    B = dbContext.Products.Where(p => p.Id % 2 != 0).AsAsyncEnumerable(), // fails: connection still open from A
});

Changes

• IAsyncEnumerableOfTConverter.OnWriteResume: Call DisposeAsync() inline when enumeration completes. If the resulting ValueTask is pending, store it as PendingTask and yield to the root serialization loop — mirroring how MoveNextAsync() pending tasks are already handled. Refactored to use a switch over the new AsyncEnumeratorState enum for clearer state transitions.
• WriteStackFrame: Replace AsyncEnumeratorIsPendingCompletion and AsyncEnumeratorIsPendingDisposal boolean fields with a single AsyncEnumeratorState enum field. Rename AsyncDisposable to AsyncEnumerator (typed as object?) to better reflect that it always stores an IAsyncEnumerator<T> instance, used for enumerator retrieval between re-entries and for exception cleanup in DisposePendingDisposablesOnExceptionAsync.
• AsyncEnumeratorState: New enum with values None, Enumerating, PendingMoveNext, and PendingDisposal to track the async enumerator's lifecycle within a stack frame.
• WriteStack: Remove CompletedAsyncDisposables field, AddCompletedAsyncDisposable(), and DisposeCompletedAsyncDisposables() — no longer needed. Update DisposePendingDisposablesOnExceptionAsync to pattern-match AsyncEnumerator on IAsyncDisposable for exception cleanup.
• JsonTypeInfoOfT.WriteHelpers.cs: Remove deferred CompletedAsyncDisposables disposal from the root serialization loop finally block.
• Build compatibility fix: Replace UnreachableException usage in IAsyncEnumerableOfTConverter with InvalidOperationException("Invalid async enumerator state.") to avoid build breaks on target frameworks where UnreachableException is unavailable.
• Tests: Add tests covering prompt disposal ordering of sibling properties, nested async enumerables, empty enumerables, truly async disposal paths, and DisposeAsync exception propagation.
• Test synchronization hardening: Update disposal-ordering tests to use explicit state-based synchronization callbacks between enumerators (instead of timing/order assertions on event list writes), reducing flakiness risk.
• Pending-then-faulted disposal coverage: Extend ThrowingDisposeAsyncEnumerable tests to also cover the async fault path (await Task.Yield(); throw ...) so the PendingDisposal suspend/resume exception propagation path is validated.

[eiriktsarpalis]

@copilot do we need to get rid of

[Copilot]

@copilot do we need to get rid of

If this is about CompletedAsyncDisposables — yes, it was already removed in the initial commit (287f2c5). If about AsyncDisposable — see my reply on the other thread; it's still needed for enumerator storage between re-entries and for exception cleanup in DisposePendingDisposablesOnExceptionAsync.

[Copilot]

Pull request overview

Fixes async JSON serialization of property-based IAsyncEnumerable<T> so enumerators are disposed promptly (and awaited when disposal is asynchronous), preventing resource leaks (e.g., DB connections) and avoiding sibling properties enumerating before prior enumerators are disposed.

Changes:

• Dispose IAsyncEnumerator<T> inline in IAsyncEnumerableOfTConverter.OnWriteResume, including supporting pending DisposeAsync() via WriteStack.PendingTask.
• Replace deferred CompletedAsyncDisposables disposal with per-frame AsyncEnumeratorState tracking (WriteStackFrame, WriteStack, root serialization loop).
• Add/extend tests for disposal ordering and exception propagation.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.AsyncEnumerable.cs	Adds tests covering disposal ordering, async disposal, empty enumerables, and disposal exceptions.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs	Replaces async-disposable tracking fields with AsyncEnumerator + AsyncEnumeratorState.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs	Removes deferred disposal list; updates exception cleanup to dispose the per-frame async enumerator when present.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.WriteHelpers.cs	Stops disposing CompletedAsyncDisposables; keeps awaiting resumable pending tasks (now includes DisposeAsync()).
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IAsyncEnumerableOfTConverter.cs	Implements inline enumerator disposal and handles pending DisposeAsync() with resumable state.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/AsyncEnumeratorState.cs	Introduces enum for async enumerator lifecycle tracking within a WriteStackFrame.
src/libraries/System.Text.Json/src/System.Text.Json.csproj	Adds new AsyncEnumeratorState.cs to compilation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

[eiriktsarpalis]

@copilot making tests depend on timing is recipe for flakiness. Rather than doing this, we want to make sure some degree of syncrhonization is performed (e.g. by having bespoke async enumerators updating state).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

[tarekgh]

I went through this change in depth (production code + the suspend/resume flow + the new tests) and it looks correct. The inline disposal in OnWriteResume, the PendingDisposal suspend/resume via state.PendingTask, the double-disposal safety (nulling AsyncEnumerator before DisposeAsync()), and the exception-cleanup walk all hold up. EndArray being written after disposal completes is fine since the base JsonCollectionConverter only writes ] when OnWriteResume returns true. Nice fix for #126695.

Two test-only items remain before this is ready:

1. The most recent review feedback on test synchronization is still open.
The disposal-ordering tests rely on await Task.Yield() plus the ordering of appends to a shared List<string> (events.IndexOf("A:Disposed") < events.IndexOf("B:MoveNext")). Per the earlier request, it would be better to have the bespoke async enumerators perform explicit state-based synchronization (e.g. the next enumerator asserting the previous one's disposed flag is already set) rather than relying on event-list ordering.

2. The pending-then-faulted DisposeAsync path is not covered.
ThrowingDisposeAsyncEnumerable.DisposeAsync() throws synchronously at the call site, so disposeTask.IsCompleted is never evaluated and the PendingDisposal suspend/resume branch is never exercised for a faulted task. The genuinely uncovered path is:

ValueTask disposeTask = enumerator.DisposeAsync();
if (!disposeTask.IsCompleted)
{
    state.PendingTask = disposeTask.AsTask();
    state.Current.AsyncEnumeratorState = AsyncEnumeratorState.PendingDisposal;
    return false;
}

followed on resume by:

case AsyncEnumeratorState.PendingDisposal:
    state.PendingTask.GetAwaiter().GetResult();

This is the trickiest path, because the exception has to survive the root loop's finally (which awaits the pending task with SuppressThrowing / catch-and-swallow) and then be re-surfaced by GetResult() on resume. A regression there (e.g. clearing the task before GetResult, or not calling it) would silently swallow disposal exceptions, and no current test would catch it.

A ThrowingDisposeAsyncEnumerable variant that does await Task.Yield(); throw ...; would cover this path, and it conveniently doubles as the bespoke async enumerator requested in item 1.

Minor: the switch cases read None -> PendingMoveNext -> PendingDisposal -> Enumerating. Putting Enumerating before PendingDisposal would match the natural enumeration lifecycle a bit more closely, though it has no behavioral impact.

[tarekgh]

Add a comment, LGTM otherwise.

[eiriktsarpalis]

@copilot please fix the build errors and address #126712 (comment)

[Copilot]

@copilot please fix the build errors and address #126712 (comment)

Addressed in 173cff5.

• Fixed the CI build break by replacing UnreachableException with InvalidOperationException in IAsyncEnumerableOfTConverter.
• Addressed Dispose IAsyncEnumerable enumerators inline during property-based JSON serialization #126712 (comment) by removing timing-based ordering assertions, adding explicit synchronization checks between enumerators, and extending DisposeAsync exception coverage to include the pending-then-faulted path (await Task.Yield(); throw ...).