JsonSerializer.DeserializeAsyncEnumerable should not wait until buffer is filled.

[eiriktsarpalis]

From #70107 (comment). The current implementation of the internal ReadFromStreamAsync method will keep reading from the stream until either the internal buffer is filled or the stream EOF's, which is intended as a performance optimization to avoid redundant deserialization calls. While this makes sense in the case of DeserializeAsync, it can impact liveness when deserializing IAsyncEnumerable sequences in examples such as the one seen in #70107.

This PR changes the behavior of stream reading specifically in DeserializeAsyncEnumerable such that the deserializer is invoked as soon as the first Stream.ReadAsync operation returns. Moreover it makes a few more changes:

• Encapsulate the buffer management logic behind the ReadBufferState struct.
• Avoid allocating a new JsonTypeInfo instance on every IAsyncEnumerator initialization.

Fix #70107.

[ghost]

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

Issue Details

---

From #70107 (comment). The current implementation of the internal ReadFromStreamAsync method will keep reading from the stream until either the internal buffer is filled or the stream EOF's, which is intended as a performance optimization to avoid redundant deserialization calls. While this makes sense in the case of DeserializeAsync, it can impact liveness when deserializing IAsyncEnumerable sequences in examples such as the one seen in #70107.

This PR changes the behavior of stream reading specifically in DeserializeAsyncEnumerable such that the deserializer is invoked as soon as the first Stream.ReadAsync operation returns. Moreover it makes a few more changes:

• Encapsulate the buffer management logic behind the ReadBufferState struct.
• Avoid allocating a new JsonTypeInfo instance on every IAsyncEnumerator initialization.

Fix #70107.

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	-

[krwq]

considering you return the copy of this buffer state in some of the methods is it safe to do it like this? Should you add some guarding against double free?

[krwq]

I'm seeing you always assign back the copy of the struct, please make sure to at least add comment on the method returning copy and the class itself. Alternative perhaps some #if DEBUG with extra code protecting us against this pattern (i.e. wrap buffer in ref type, set it to null on Dispose and check with Debug.Assert)

[eiriktsarpalis]

considering you return the copy of this buffer state in some of the methods is it safe to do it like this? Should you add some guarding against double free?

Yes, assuming you somehow end up disposing two copies of the same struct then that is a possibility, but we're not doing this in the handful of methods that creates the struct.

perhaps some #if DEBUG extra code protecting us against this pattern (i.e. wrap buffer in ref type, set it to null on Dispose and check with Debug.Assert)

Can you give an example? Not sure I understand. FWIW this is not new code, I'm just moving existing logic to the struct.

[krwq]

ReadBufferState a = ...;
ReadBufferState b = a;
a.Dispose();
b.Dispose(); // double free or usage after dispose if we don't dispose and continue using

[eiriktsarpalis]

I understand the condition, my question is what assert could guard against such a potential scenario.

[krwq]

in the struct you could potentially add nested class like this (under #if DEBUG):

private class BufferRef
{
  public byte[]? Buffer;
}

and then add field in the struct:

BufferRef _myBuffer = new BufferRef(); // assign _buffer to _myBuffer.Buffer;

then if struct gets copied then ref gets copied as well but underlying field will be shared. Then in the Dispose you could clear that field and check if it's not null.

[krwq]

as per discussion offline, better to do this for all similar structs in separate PR in order to potentially find potential memory corruption bug