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

Periodically flush when using PipeWriter in Json #102541

Merged
merged 9 commits into from
May 28, 2024

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented May 22, 2024

Makes PooledByteBufferWriter implement PipeWriter and uses PipeWriter.UnflushedBytes to check whether resumable json converters should let the bytes be flushed.

Fixes #66102 and unblocks dotnet/aspnetcore#55740

Didn't remove IAsyncSerializationBufferWriterContext yet because it would require checking BufferWriter is IDisposable and passing the FlushThreshold value into the serialize functions.

We throw if PipeWriter.CanGetUnflushedBytes == false and state.FlushThreshold > 0 as it could lead to excess buffering for large json payloads. We could consider giving users a way to opt-in to the bad behavior in the future. But currently there is no way to set state.FlushThreshold in the PipeWriter case yet.

@eiriktsarpalis
Copy link
Member

Didn't remove IAsyncSerializationBufferWriterContext yet because it would require checking BufferWriter is IDisposable and passing the FlushThreshold value into the serialize functions.

I would recommend removing it, as it doesn't really serve purpose anymore. checking if the writer is IDisposable isn't a huge deal and if that isn't acceptable you could just expose a second IDisposable? disposable parameter on the common workhorse method. FlushThreshold could just be a separate parameter.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@eiriktsarpalis
Copy link
Member

One of the new tests seems to have failed once in CI.

@davidfowl
Copy link
Member

This change looks really good!

@BrennanConroy BrennanConroy merged commit 010a744 into dotnet:main May 28, 2024
83 checks passed
@BrennanConroy BrennanConroy deleted the brecon/pipeflush branch May 28, 2024 23:39
@BrennanConroy BrennanConroy added this to the 9.0.0 milestone May 28, 2024
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants