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

Add async serialization and deserialization support #2643

Open
wants to merge 4 commits into
base: release/13.0.1
Choose a base branch
from

Conversation

atykhyy
Copy link

@atykhyy atykhyy commented Jan 24, 2022

This PR adds async serialization and deserialization support to Newtonsoft.Json (#1193). It uses a Roslyn source generator to generate async methods from their synchronous counterparts at build time. There is therefore no double code maintenance burden. Generated source is checked in for reference, easier debugging and PDB source links.

Async code paths are tested by the existing code suite by means of substituting calls to async methods at three key entry points - JsonSerializer.SerializeInternal, JsonSerializer.PopulateInternal and JsonSerializer.DeserializeInternal - when TEST_ASYNC_AS_SYNC preprocessor constant is defined. Completeness of async code generation coverage is ensured by wrapping JsonReader and JsonWriter objects passed into these methods with wrappers which throw exceptions from all sync I/O methods. When TEST_ASYNC_AS_SYNC is not defined, tests use existing (synchronous) code paths. All tests pass in this mode.

At the moment the PR is in draft stage. The main project is set up to always test the async code paths. Over 90% of the test suite passes in this mode, but tests that rely on custom converters in Newtonsoft.Json.Converters namespace fail because I haven't yet extended them with async implementations. I will add these if this PR has a chance of being merged.

There are a few points where I had questions, marked with comments with the XXX tag. I have used Task rather than ValueTask for simplicity, but it is easy to switch to ValueTask for greater performance and much fewer allocations provided that it is acceptable to introduce a dependency on the System.Threading.Task.Extensions nuget package.

@sungam3r
Copy link

👍 I am amazed by people who continue attempts to invest in this project! On the other hand, it's a shame that the work performed with a probability is close to zero will ever get to the release.

@atykhyy atykhyy marked this pull request as ready for review June 23, 2022 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants