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.

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

Add PipeWriter overloads to Json #101461

Merged
merged 18 commits into from
May 13, 2024
Merged

Add PipeWriter overloads to Json #101461

merged 18 commits into from
May 13, 2024

Conversation

BrennanConroy
Copy link
Member

Adds PipeWriter part of #68586.
Also closes #28760.

No new unique tests were added so far, just hooked into existing test groups. Could definitely add a few new tests centered around writes that span buffers, as well as partial writes. Pointers to where those should live, and if there are tests like that would be helpful.

@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

This was referenced Apr 24, 2024
Closed
Closed
Closed
This was referenced Apr 24, 2024
Open
Closed
Closed
Closed
eiriktsarpalis
eiriktsarpalis reviewed Apr 24, 2024
View reviewed changes
src/libraries/NetCoreAppLibrary.props
@@ -220,7 +221,6 @@
Microsoft.Extensions.Options.DataAnnotations;
Microsoft.Extensions.Primitives;
System.Diagnostics.EventLog;
System.IO.Pipelines;
Copy link
Member

Choose a reason for hiding this comment

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

What do these properties do, out of curiosity?

Copy link
Member

@ViktorHofer ViktorHofer Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This property makes the assembly get included in the transport package for aspnetcore. Aspnetcore includes everything from that transport package in their shared framework.

Given that you move this assembly inbox into Microsoft.NETCore.App, this change is correct.

@eiriktsarpalis
Copy link
Member

Could definitely add a few new tests centered around writes that span buffers, as well as partial writes. Pointers to where those should live, and if there are tests like that would be helpful.

I think this warrants a new test class, probably a set of files that sit next to Stream.WriteTests.cs and Stream.ReadTests.cs. Echoing that naming convention, perhaps Pipes.WriteTests.cs?

@wtgodbe wtgodbe mentioned this pull request Apr 24, 2024
Closed
@build-analysis build-analysis bot mentioned this pull request Apr 24, 2024
Closed
This was referenced May 9, 2024
Open
Open
eiriktsarpalis
eiriktsarpalis approved these changes May 10, 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! Any changes you need to make on your side before I get this merged?

@BrennanConroy
Copy link
Member Author

Thanks! Any changes you need to make on your side before I get this merged?

Nope! All good here 😄

eiriktsarpalis and others added 2 commits May 11, 2024 19:29
@eiriktsarpalis @davidfowl
Update src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Se…
6a6d91b 
…rialization/Pipe.WriteTests.cs

Co-authored-by: David Fowler <davidfowl@gmail.com>
@eiriktsarpalis
Merge branch 'main' into brecon/pipe
5f622c8
This was referenced May 12, 2024
Open
Open
Open
Open
davidfowl
davidfowl approved these changes May 12, 2024
View reviewed changes
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Small nits

@eiriktsarpalis eiriktsarpalis merged commit 741af4a into dotnet:main May 13, 2024
83 checks passed
@BrennanConroy BrennanConroy deleted the brecon/pipe branch May 13, 2024 15:58
@eiriktsarpalis
Copy link
Member

@wtgodbe heads up, the PR just got merged.

This was referenced May 16, 2024
Closed
Closed
@kotlarmilos kotlarmilos mentioned this pull request May 22, 2024
Closed
halter73
halter73 reviewed May 22, 2024
View reviewed changes
...System.Text.Json/src/System/Text/Json/Serialization/AsyncSerializationBufferWriterContext.cs
ThrowHelper.ThrowOperationCanceledException_PipeWriteCanceled();
}

ThrowHelper.ThrowOperationCanceledException_PipeWriteCompleted();
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to throw in this case? I think this is going to end up throwing a lot of unwanted exceptions.

For normal HTTP/1.1 response bodies, this just means that the client has closed the connection. BodyWriter.FlushAsync with return a completed flush result, but Body.FlushAsync would not throw because it doesn't want to create a bunch of unnecessary noise in applications writing small responses. Things that actually care about connection close like the static files middleware subscribe to RequestAborted.

Copy link
Member

Choose a reason for hiding this comment

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

I think you are right @halter73. Let's remove this exception in a follow up PR.

@halter73 halter73 mentioned this pull request May 23, 2024
Merged
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@BrennanConroy @eiriktsarpalis
@davidfowl @Ruihan-Yin
Add PipeWriter overloads to Json (dotnet#101461)
41d981a 
* Add PipeWriter overloads to Json

* test

* organize and tests

* cleanup

* FlushThreshold

* cleanup

* system.buffer is special

* tests

* fixup

* de-dupe

* minor

* fb

* no async!

* Update src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Pipe.WriteTests.cs

Co-authored-by: David Fowler <davidfowl@gmail.com>

---------

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Co-authored-by: David Fowler <davidfowl@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wtgodbe wtgodbe wtgodbe left review comments

@ericstj ericstj ericstj left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@davidfowl davidfowl davidfowl approved these changes

@halter73 halter73 halter73 left review comments

Assignees

@BrennanConroy BrennanConroy

Labels
area-System.Text.Json new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider moving System.IO.Pipelines into the shared framework
7 participants
@BrennanConroy @eiriktsarpalis @halter73 @davidfowl @ViktorHofer @ericstj @wtgodbe