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

Don't throw Exceptions from PipeWriter APIs after RST_STREAM #11675

Merged
merged 11 commits into from
Jun 28, 2019
Merged

Don't throw Exceptions from PipeWriter APIs after RST_STREAM #11675

merged 11 commits into from
Jun 28, 2019

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Jun 28, 2019 •
edited
Loading

@halter73 halter73 force-pushed the halter73/allow-writes-post-http2-abort branch 2 times, most recently from 015382c to f283b35 Compare June 28, 2019 06:25
@halter73 halter73 requested a review from davidfowl June 28, 2019 06:25
@halter73
Copy link
Member Author

By fixing the tests for this PR, I'm pretty sure I also accidentally fixed #7370.

@halter73 halter73 force-pushed the halter73/allow-writes-post-http2-abort branch 3 times, most recently from 7f27446 to 785bb36 Compare June 28, 2019 10:14
jkotalik
jkotalik reviewed Jun 28, 2019
View reviewed changes
src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs
@@ -360,6 +410,25 @@ private async ValueTask<FlushResult> ProcessDataWrites()
return flushResult;
}

private Memory<byte> GetFakeMemory(int sizeHint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should consider a shared output producer base class as there are now quite a few methods that are "shared".

Copy link
Member Author

Choose a reason for hiding this comment

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

There's definitely some potential for better reuse. It could be a nice refactoring/cleanup down the road. I don't think it's critical for this change as this only duplicates a little more than we already were, and the logic that was copied is quite simple.

jkotalik
jkotalik reviewed Jun 28, 2019
View reviewed changes
src/Servers/Kestrel/test/InMemory.FunctionalTests/ResponseTests.cs

await Task.CompletedTask;
writeEx = Assert.Throws<InvalidOperationException>(() => httpContext.Response.BodyWriter.GetMemory());
Copy link
Contributor

Choose a reason for hiding this comment

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

This part confuses me. I thought we wanted to have the same behavior as streams where calling WriteAsync after Complete noops. However, I think the difference is that the noop behavior only occurs when the connection was aborted or the streams go away. Hence why you added the fake memory to HTTP2?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want the same behavior as Streams where aborting http connection or http/2 stream makes writing no-op. If the application explicitly completes the PipeWriter, that a different story entirely.

jkotalik
jkotalik approved these changes Jun 28, 2019
View reviewed changes
Copy link
Contributor

@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.

Do we need an explicit test for not throwing an InvalidOperationException after abort?

Tratcher
Tratcher requested changes Jun 28, 2019
View reviewed changes
src/Servers/Kestrel/Core/src/Internal/Http/HttpResponsePipeWriter.cs
@@ -35,7 +37,7 @@ public override void CancelPendingFlush()
public override void Complete(Exception exception = null)
{
ValidateState();
_pipeControl.Complete(exception);
_completeTask = _pipeControl.CompleteAsync(exception);
Copy link
Member

Choose a reason for hiding this comment

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

This sets us up for an inconsistent / confusing end-to-end story. Let's go over that before continuing here.
#7370 (comment)

@analogrelay analogrelay added this to the 3.0.0-preview7 milestone Jun 28, 2019
@halter73 halter73 force-pushed the halter73/allow-writes-post-http2-abort branch from 785bb36 to 68b85c1 Compare June 28, 2019 19:09
@halter73 halter73 force-pushed the halter73/allow-writes-post-http2-abort branch from ae8cd9a to 36ba5de Compare June 28, 2019 20:03
@jkotalik jkotalik mentioned this pull request Jun 28, 2019
Merged
@halter73 halter73 changed the title Don't throw InvalidOperationExceptions from PipeWriter APIs after HTTTP/2 aborts Don't throw Exceptions from PipeWriter APIs after RST_STREAM Jun 28, 2019
@halter73 halter73 merged commit bcb59fa into master Jun 28, 2019
@ghost ghost deleted the halter73/allow-writes-post-http2-abort branch June 28, 2019 21:40
@halter73 halter73 mentioned this pull request Jul 2, 2019
Merged
@halter73 halter73 mentioned this pull request Sep 22, 2020
Closed
@amcasey amcasey added 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
Reviewers

@davidfowl davidfowl davidfowl left review comments

@Tratcher Tratcher Tratcher approved these changes

@jkotalik jkotalik jkotalik approved these changes

@analogrelay analogrelay Awaiting requested review from analogrelay

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
3.0.0-preview7
Development

Successfully merging this pull request may close these issues.
7 participants
@halter73 @davidfowl @Tratcher @jkotalik @analogrelay @Pilchie @amcasey