Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix System.Text.Json IAsyncEnumerator disposal on cancellation #57505

Conversation

eiriktsarpalis
Copy link
Member

Addresses an issue in System.Text.Json IAsyncEnumerable serialization where, in the event of a user-thrown exception, the serializer will attempt to dispose the IAsyncEnumerator instance without making sure that there are no pending MoveNextAsync() operations. In the case of compiler-generated IAsyncEnumerables, this can result in a NotSupportedException being thrown, see #51176. This issue will most typically manifest in cases where the ambient cancellation token has fired, but authors of the serialized IAsyncEnumerable are not passing that cancellation token to nested async operations.

This PR changes the IAsyncEnumerable implementation to wait on any pending MoveNextAsync() operations even if an exception has been thrown. This should not have any impact on latency provided that users take care to pass the IAsyncEnumerator cancellation token to any underlying dependencies. It also fixes an unrelated issue where in certain exceptional conditions the serializer would try to dispose the IAsyncEnumerator twice.

Fix #57360.

cc @davidfowl

…Disposable instances are disposed exactly once.

Fixes dotnet#57360.
@ghost
Copy link

ghost commented Aug 16, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Addresses an issue in System.Text.Json IAsyncEnumerable serialization where, in the event of a user-thrown exception, the serializer will attempt to dispose the IAsyncEnumerator instance without making sure that there are no pending MoveNextAsync() operations. In the case of compiler-generated IAsyncEnumerables, this can result in a NotSupportedException being thrown, see #51176. This issue will most typically manifest in cases where the ambient cancellation token has fired, but authors of the serialized IAsyncEnumerable are not passing that cancellation token to nested async operations.

This PR changes the IAsyncEnumerable implementation to wait on any pending MoveNextAsync() operations even if an exception has been thrown. This should not have any impact on latency provided that users take care to pass the IAsyncEnumerator cancellation token to any underlying dependencies. It also fixes an unrelated issue where in certain exceptional conditions the serializer would try to dispose the IAsyncEnumerator twice.

Fix #57360.

cc @davidfowl

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Aug 16, 2021
@eiriktsarpalis eiriktsarpalis self-assigned this Aug 16, 2021
@eiriktsarpalis
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@eiriktsarpalis
Copy link
Member Author

/azp list

@eiriktsarpalis
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eiriktsarpalis
Copy link
Member Author

Failing tests are outerloop networking tests and a few that are already occurring in main, possibly related to the changes in #57453 and #55926.

@eiriktsarpalis eiriktsarpalis merged commit 61075fb into dotnet:main Aug 17, 2021
@eiriktsarpalis eiriktsarpalis deleted the fix-asyncenumerable-cancellationtoken-disposal branch August 17, 2021 10:16
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants