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

Expose response PipeWriter in Kestrel #7110

Merged
merged 33 commits into from
Feb 9, 2019
Merged

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik commented Jan 29, 2019

For #4351 and does many other things too that are required for this change.

  • Exposes the PipeWriter as directly as possible from Kestrel (for both Http1 and Http2)
    • Pay careful attention to Http1 chunking logic as that actually required work.
  • Changes most tests to start using the PipeWriter

Things left to do:

  • Add tests for setting response body/ response pipe and how they override each other.
  • Perf tests (micro benchmarks and before/after with benchmarks).

Have fun everyone 😄

Copy link
Contributor Author

@jkotalik jkotalik left a comment

Choose a reason for hiding this comment

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

Some feedback for myself and things to figure out.

@jkotalik
Copy link
Contributor Author

Looks like swapping WriteAsync broke stuff too. I think they are issues with StreamPipeWriter so I'll investigate and create a separate PR.

@JunTaoLuo
Copy link
Contributor

👀

@sebastienros
Copy link
Member

/AzurePipelines run AspNetCore-benchmarking-pr

@azure-pipelines
Copy link

Successfully queued 1 pipeline(s).

@jkotalik
Copy link
Contributor Author

Figured out why all response compression tests are failing. We are now double flushing the stream for ResponseCompression; once from StreamPipeWriter.FlushAsync, and another from BodyWrapperStream.FinishCompressionAsync (which calls Dispose), which causes extra compression frames to be written to the response.

To fix this, either we need a way to either make the StreamPipeWriter not call FlushAsync on the underlying stream and only write the buffered contents to the stream (which may straight up break scenarios), or we need to fix the tests to expect a longer length (which isn't desired as it means response compression is less optimized).

@Tratcher
Copy link
Member

Go ahead and update the response compression tests (after verifying the data isn't corrupt and can round trip). An extra flush at the end isn't the end of the world. We do want to discourage frequent flushes though.

@jkotalik
Copy link
Contributor Author

jkotalik commented Feb 5, 2019

🆙 📅 . There is still some crust around the new Disposal logic I added, but I wanted to make sure it's a good idea before cleaning up.

@jkotalik
Copy link
Contributor Author

jkotalik commented Feb 8, 2019

Bad merge. Fixing.

@benaadams
Copy link
Member

1 error on AspNetCore-helix-test but only on Centos.7.Amd64.Open

[xUnit.net 00:00:02.41]     Microsoft.AspNetCore.Components.Test.RendererTest.CanTriggerEventHandlerDisposedInEarlierPendingBatchAsync [FAIL]
Failed   Microsoft.AspNetCore.Components.Test.RendererTest.CanTriggerEventHandlerDisposedInEarlierPendingBatchAsync
Error Message:
 Assert.Throws() Failure
Expected: typeof(System.ArgumentException)
Actual:   (No exception was thrown)
Stack Trace:
   at Microsoft.AspNetCore.Components.Test.RendererTest.CanTriggerEventHandlerDisposedInEarlierPendingBatchAsync() 
in /_/src/Components/Components/test/RendererTest.cs:line 1364
--- End of stack trace from previous location where exception was thrown ---

@jkotalik
Copy link
Contributor Author

jkotalik commented Feb 9, 2019

Fortunately Helix tests aren't required yet 😄

@jkotalik jkotalik merged commit 35b99e4 into master Feb 9, 2019
@jkotalik jkotalik deleted the jkotalik/kestrelResponsePipe branch February 9, 2019 01:24
martincostello added a commit to martincostello/aspnetcore that referenced this pull request Feb 9, 2019
Remove local development item group accidentally added by dotnet#7110.
jkotalik pushed a commit that referenced this pull request Feb 9, 2019
Remove local development item group accidentally added by #7110.
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.