Skip to content

System.Text.Json: Support for IAsyncEnumerable #37981

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

Closed

Conversation

logiclrd
Copy link
Contributor

@logiclrd logiclrd commented Jun 16, 2020

This is my first pass at support for IAsyncEnumerable in System.Text.Json, which should satisfy issues #1569 and #1570 (except that, as commented in #1570, I don't think the deserialization can actually be asynchronous, because the object graph might have multiple IAsyncEnumerables in it, and each one would need its own pointer into the underlying stream).

This is my first time doing any work with C# 8, and also my first time doing any work with the .NET codebase, so I'm fully expecting there to be some things I overlooked, both in style and substance. :-)

My implementation builds upon IEnumerableDefaultConverter, which already has logic for reading a JSON array and passing the elements off to a collection that the subclass gets to define.

In addition to this, the converter needs to stash an IAsyncEnumerator between invocations. In the case of reference types, it can store this directly into WriteStackFrame using type covariance. For value types, type covariance isn't an option. So, two specializations of the IAsyncEnumerableOfTConverter are provided, customizing how the IAsyncEnumerator is stashed, using a simple wrapper for value types.

When deserializing into a field whose type is IAsyncEnumerable<T>, some class that implements the interface is needed, and I'm not aware of one built into the framework, so a simple AsyncEnumerableList that subclasses List<T> and adds an implementation of IAsyncEnumerable<T> (just synchronous pass-through, since the data is already there) is used.

I modeled the way converter objects get created after what I saw for IEnumerableOfT, with a factory that I register in sequence before the IEnumerableConverterFactory, on the assumption that if a class chooses to implement both, it probably would prefer that the async variant be used.

Finally, I added a handful of unit tests in new file IAsyncEnumerableTests.cs to capture the various wrinkles I could think of.

…eDefaultConverter with concrete implementations IAsyncEnumerableOfTObjectConverter and IAsyncEnumerableOfTValueConverter, where the latter has to do a little bit of extra wrapping to store the IAsyncEnumerator, because type covariance can't be used with value types. Added field AsyncEnumerator to WriteStackFrame.

Added class AsyncEnumerableList to provide a simple list type with a dummy implementation of IAsyncEnumerable, to allow IAsyncEnumerable values to be populated during deserialization.
Added class IAsyncEnumerableConverterFactory to automatically inject appropriate generic arguments into constructed instances of IAsyncEnumerableOfTObjectConverter and IAsyncEnumerableOfTValueConverter.
Updated JsonClassInfo.cs to check for objects implementing IAsyncEnumerable and extract their element type.
Added IAsyncEnumerableConverterFactory to the list of default factories registered in JsonSerializerOptions.Converters.cs.
Added unit tests in new file IAsyncEnumerableTests.cs in System.Text.Json.Tests.
@layomia layomia added this to the 5.0.0 milestone Jun 17, 2020
@layomia layomia self-assigned this Jun 17, 2020
@layomia
Copy link
Contributor

layomia commented Jun 17, 2020

Thanks for the PR.

This feature needs to go through API review to verify if we want to take this approach, or something similar to the API suggestion in #1570 (comment). See the API review process here.

One thing that can move this forward is to consolidate your notes on #1570 into a single proposal, and compare the approach to #1570 (comment). Some code samples showing the API usage would be good as well.

@layomia layomia closed this Jun 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants