Skip to content

Periodically flush when using PipeWriter in Json #102541

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
May 28, 2024
Merged

Periodically flush when using PipeWriter in Json #102541

merged 9 commits into from
May 28, 2024

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented May 22, 2024 •
edited
Loading

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.

eiriktsarpalis
eiriktsarpalis approved these changes May 23, 2024
View reviewed changes
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.

@build-analysis build-analysis bot mentioned this pull request May 24, 2024
Open
@build-analysis build-analysis bot mentioned this pull request May 24, 2024
Closed
@davidfowl
Copy link
Member

This change looks really good!

@BrennanConroy BrennanConroy merged commit 010a744 into dotnet:main May 28, 2024
@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
@BrennanConroy BrennanConroy mentioned this pull request May 28, 2024
Closed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@BrennanConroy @Ruihan-Yin
Periodically flush when using PipeWriter in Json (dotnet#102541)
3a824a8
@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.

Reviewers

@halter73 halter73 halter73 left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@davidfowl davidfowl davidfowl approved these changes

Assignees

@BrennanConroy BrennanConroy

Labels

area-System.Text.Json

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Passing a Stream Utf8JsonWriter to a JsonSerializer.Serialize method results in Pending bytes being misreported

4 participants

@BrennanConroy @eiriktsarpalis @davidfowl @halter73