Use buffer streams to solve the problem with synchronous reads.

[SebastianStehle]

Hi, I copied the approach from ASP.NET Core Input and Output Formatter to solve the problem for synchronous reads with Newtonsoft.JSON.

I also updated the server project to test both implementations.

[sungam3r]

I think that it would be better to rebase to develop instead of master.

[sungam3r]

I mean we dropped too much code from master preparing v6.

[Shane32]

Here's what I'm gathering/thinking:

Newtonsoft is synchronous instead of asynchronous, and FileBufferingReadStream allows to read the whole stream asynchronously, then deserialize the in-memory stream. This could be done easily with a MemoryStream, but in this case FileBufferingReadStream is designed to buffer to disk if the size of the buffered data exceeds a preset amount, like 32kb. Seems to make sense, especially since the size of a graphql query should rarely be over 32kb.

For writing from Newtonsoft, we already have a HttpResponseStreamWriter optimization which buffers writes, but does not buffer to disk but synchronously writes periodically. It seems to be working fine, although for a truly asynchronous implementation, one would need to use something like FileBufferingWriteStream. I really don't know whether it is better to have synchronous writes and not buffer to disk, or buffer to disk but have all async calls. In any case, either option doesn't seem to be a bad option. One more thought: with the FileBufferingWriteStream, we wouldn't need to require users to enable synchronous reading/writing in ASP.Net Core.

As such, it seems to me that the immediate issue is really more of reading than writing for the current version of GraphQL server. If we do a fix for the current version, I might simply limit it to using FileBufferingReadStream. This should be easy for the current version as the read serializer exists within the server project, and is only used by the http middleware.

For the next version of GraphQL, the serializers are in the main project. We should consider if we should use HttpResponseStreamReader and HttpResponseStreamWriter, or FileBufferingReadStream and FileBufferingWriteStream. If the former, then synchronous reads/writes are necessary. If the latter, then we may be often be double buffering in the case of reading from or writing to a MemoryStream. It's also noteworthy that the code would need to be copied in for either case, as the serializer does not hold a reference to ASP.Net Core.

There is one final option for the next version -- remove both options above, and instead move the buffering code into the middleware. However, it should only be used for Newtonsoft, as System.Text.Json does not need any buffering.

I'm not sure there is a clear answer here. The answer that works for the most situations is probably to implement HttpResponseStreamReader for the next version.

[SebastianStehle]

Newtonsoft is synchronous instead of asynchronous

System.Text.JSON is also synchronous. They just solved it in the API directly by using a in-memory buffer.

For the next version of GraphQL, the serializers are in the main project. We should consider if we should use HttpResponseStreamReader and HttpResponseStreamWriter, or FileBufferingReadStream and FileBufferingWriteStream. If the former, then synchronous reads/writes are necessary. If the latter, then we may be often be double buffering in the case of reading from or writing to a MemoryStream. It's also noteworthy that the code would need to be copied in for either case, as the serializer does not hold a reference to ASP.Net Core.

In ASP.NET Core they actually do both together. HttpResponseStreamReader + FileBufferingReadStream. I guess they just measured it and the original StreamReader was too slow. It is strange because StreamReader also has a buffer. My guess is that they wanted to use a bigger buffer, but the StreamReader does not use an ArrayPool and therefore they introduced a wrapper to reduce allocations.

I think that it would be better to rebase to develop instead of master.

Tried that, but does not build locally, because it is looking for preview packages in develop:

Unable to find package GraphQL.DataLoader with version (>= 5.0.0-preview-476)

[sungam3r]

You may download each preview version from here https://github.com/orgs/graphql-dotnet/packages?repo_name=graphql-dotnet

[Shane32]

@sungam3r I suggest we add a readonly property to IGraphQLSerializer called SupportsAsync -- or two, with one called SupportsAsyncReads and the other SupportsAsyncWrites. Something like that so that we can know whether we should apply buffer streams in the server project. I'm pretty sure we would/should not when using System.Text.Json as it supports asynchronous reads and writes. Unfortunately adding such a property to an interface is a breaking change, so it would need to be in GraphQL v6. Alternatively we could write a v5 patch, like a pseudo-interface like IGraphQLAsyncSerializer but I would not be inclined to do that.

[sungam3r]

It looks a bit weird to add properties like SupportsAsyncReads and SupportsAsyncWrites whereas IGraphQLSerializer already has methods ReadAsync and WriteAsync 😕 though you thoughts is understandable.

[Shane32]

It looks a bit weird to add properties like SupportsAsyncReads and SupportsAsyncWrites whereas IGraphQLSerializer already has methods ReadAsync and WriteAsync 😕 though you thoughts is understandable.

Let's decide if adding some type of property like IsNativelyAsync would be good to add to IGraphQLSerializer for v7. No need to add any other optimizations now, but if we add a single property, it will give us the opportunity to do so at our leisure.

// IGraphQLSerializer
bool IsNativelyAsync { get; }

// GraphQL.NewtonsoftJson
public IsNativelyAsync => false;

// GraphQL.SystemTextJson
public IsNativelyAsync => true;

[sungam3r]

OK. Perhaps this is better than nothing.