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

Handle client FIN as request abort #1218

Merged
merged 1 commit into from Dec 13, 2016

Conversation

Projects
None yet
4 participants
@cesarbs
Contributor

cesarbs commented Nov 18, 2016

#1139

This part of the change is mostly to avoid test failures in HttpsTests.EmptyRequestLoggedAsInformation. After the change to handle FIN as abort, this test started failing consistently because the OnConnectionAsync continuation would run after the abort had taken place, the connection had ended and the server had been disposed - at that point ReadInputAsync would try to lease a block from an already disposed memory pool, triggering the assert and failing the entire test run.

@halter73 @Tratcher @pakrym

Show outdated Hide outdated test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs
@@ -533,7 +533,7 @@ public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIg
"GET / HTTP/1.1",
"",
"");
await connection.ReceiveEnd(
await connection.ReceiveForcedEnd(

This comment has been minimized.

@halter73

halter73 Nov 21, 2016

Member

I see a lot of ReceiveEnd to ReceiveForcedEnd changes. In tests like these, I'm not exactly sure why it's necessary.

@halter73

halter73 Nov 21, 2016

Member

I see a lot of ReceiveEnd to ReceiveForcedEnd changes. In tests like these, I'm not exactly sure why it's necessary.

This comment has been minimized.

@cesarbs

cesarbs Nov 28, 2016

Contributor

Those are tests where we're expecting the server to close the connection without the client having sent a FIN.

@cesarbs

cesarbs Nov 28, 2016

Contributor

Those are tests where we're expecting the server to close the connection without the client having sent a FIN.

This comment has been minimized.

@halter73

halter73 Nov 28, 2016

Member

Wouldn't ReceiveEnd send the FIN after it received all the expected data now? I expect ReceiveForcedEnd would still take a while since it's waiting for the connection to close with an IOException or otherwise. Unlike ReceiveEnd, ReceiveForcedEnd hasn't been modified to send a FIN.

@halter73

halter73 Nov 28, 2016

Member

Wouldn't ReceiveEnd send the FIN after it received all the expected data now? I expect ReceiveForcedEnd would still take a while since it's waiting for the connection to close with an IOException or otherwise. Unlike ReceiveEnd, ReceiveForcedEnd hasn't been modified to send a FIN.

This comment has been minimized.

@halter73

halter73 Nov 28, 2016

Member

My bad. I didn't notice that this test expected the server to close the connection because of a buggy app.

@halter73

halter73 Nov 28, 2016

Member

My bad. I didn't notice that this test expected the server to close the connection because of a buggy app.

Show outdated Hide outdated test/shared/TestConnection.cs
Show outdated Hide outdated test/shared/TestConnection.cs
@@ -420,6 +422,42 @@ public void CanUpgradeRequestWithConnectionKeepAliveUpgradeHeader()
}
}
[Fact]
public async Task RequestAbortedTokenFiredOnClientFIN()

This comment has been minimized.

@halter73

halter73 Nov 21, 2016

Member

Do we have a test verifying the RequestAborted token isn't fired for the last successful request made before the connection is closed?

@halter73

halter73 Nov 21, 2016

Member

Do we have a test verifying the RequestAborted token isn't fired for the last successful request made before the connection is closed?

This comment has been minimized.

@cesarbs

cesarbs Nov 29, 2016

Contributor

Added to FrameTests.

@cesarbs

cesarbs Nov 29, 2016

Contributor

Added to FrameTests.

@cesarbs

This comment has been minimized.

Show comment
Hide comment
@cesarbs

cesarbs Nov 29, 2016

Contributor

@halter73 Added code to reset the RequestAborted token before the last write in each response.

Contributor

cesarbs commented Nov 29, 2016

@halter73 Added code to reset the RequestAborted token before the last write in each response.

public Task ProduceEndAsync()
{
return ProduceEnd();

This comment has been minimized.

@cesarbs

cesarbs Nov 29, 2016

Contributor

@halter73 Any strong feelings about doing this vs. just exposing ProduceEnd as public?

@cesarbs

cesarbs Nov 29, 2016

Contributor

@halter73 Any strong feelings about doing this vs. just exposing ProduceEnd as public?

This comment has been minimized.

@halter73

halter73 Nov 29, 2016

Member

No strong feelings.

Checking for the last write is pretty ugly, but not quite as bad as I thought. I wonder if we could get a micro benchmark for writing responses. Maybe that could use MockLibuv for now.

@halter73

halter73 Nov 29, 2016

Member

No strong feelings.

Checking for the last write is pretty ugly, but not quite as bad as I thought. I wonder if we could get a micro benchmark for writing responses. Maybe that could use MockLibuv for now.

@cesarbs

This comment has been minimized.

Show comment
Hide comment
@cesarbs

cesarbs Nov 30, 2016

Contributor

Updated.

Contributor

cesarbs commented Nov 30, 2016

Updated.

@cesarbs

This comment has been minimized.

Show comment
Hide comment
@cesarbs

cesarbs Dec 5, 2016

Contributor

Ping.

Contributor

cesarbs commented Dec 5, 2016

Ping.

@pakrym

This comment has been minimized.

Show comment
Hide comment
@pakrym

pakrym Dec 5, 2016

Contributor

We might want to perf test this because we are adding another check that is per-write.

Contributor

pakrym commented Dec 5, 2016

We might want to perf test this because we are adding another check that is per-write.

@cesarbs

This comment has been minimized.

Show comment
Hide comment
@cesarbs

cesarbs Dec 6, 2016

Contributor

No impact on plaintext benchmark:

dev PR
1173992.5 RPS 1170175.3

Will work on microbenchmark.

Contributor

cesarbs commented Dec 6, 2016

No impact on plaintext benchmark:

dev PR
1173992.5 RPS 1170175.3

Will work on microbenchmark.

@cesarbs

This comment has been minimized.

Show comment
Hide comment
@cesarbs

cesarbs Dec 6, 2016

Contributor

Fixed a race in ResponseTests.ResponseBodyNotWrittenOnHeadResponseAndLoggedOnlyOnce.

Contributor

cesarbs commented Dec 6, 2016

Fixed a race in ResponseTests.ResponseBodyNotWrittenOnHeadResponseAndLoggedOnlyOnce.

@cesarbs

This comment has been minimized.

Show comment
Hide comment
@cesarbs

cesarbs Dec 10, 2016

Contributor

Write() microbenchmarks

dev

Method Mean StdDev Gen 0 Allocated
Write 39.8272 ns 0.4220 ns - 0 B
WriteChunked 40.1696 ns 0.5210 ns - 0 B
WriteAsync 76.2597 ns 0.7999 ns 0.0068 63 B
WriteAsyncChunked 76.5476 ns 0.8251 ns 0.0070 63 B
WriteAsyncAwaited 110.1888 ns 0.3927 ns 0.0037 63 B
WriteAsyncAwaitedChunked 111.9686 ns 1.3847 ns 0.0033 63 B
ProduceEnd 35.1201 ns 0.4583 ns 0.0086 63 B
ProduceEndChunked 35.3245 ns 0.3355 ns 0.0084 63 B

This change

Method Mean StdDev Gen 0 Allocated
Write 57.9786 ns 0.1677 ns - 0 B
WriteChunked 58.1488 ns 0.1471 ns - 0 B
WriteAsync 91.0255 ns 0.2082 ns 0.0033 63 B
WriteAsyncChunked 93.8709 ns 1.3749 ns 0.0034 63 B
WriteAsyncAwaited 126.9702 ns 0.8035 ns 0.0034 63 B
WriteAsyncAwaitedChunked 129.1539 ns 2.0402 ns 0.0040 63 B
ProduceEnd 36.8412 ns 1.3556 ns 0.0085 63 B
ProduceEndChunked 35.2432 ns 0.2778 ns 0.0086 63 B
Contributor

cesarbs commented Dec 10, 2016

Write() microbenchmarks

dev

Method Mean StdDev Gen 0 Allocated
Write 39.8272 ns 0.4220 ns - 0 B
WriteChunked 40.1696 ns 0.5210 ns - 0 B
WriteAsync 76.2597 ns 0.7999 ns 0.0068 63 B
WriteAsyncChunked 76.5476 ns 0.8251 ns 0.0070 63 B
WriteAsyncAwaited 110.1888 ns 0.3927 ns 0.0037 63 B
WriteAsyncAwaitedChunked 111.9686 ns 1.3847 ns 0.0033 63 B
ProduceEnd 35.1201 ns 0.4583 ns 0.0086 63 B
ProduceEndChunked 35.3245 ns 0.3355 ns 0.0084 63 B

This change

Method Mean StdDev Gen 0 Allocated
Write 57.9786 ns 0.1677 ns - 0 B
WriteChunked 58.1488 ns 0.1471 ns - 0 B
WriteAsync 91.0255 ns 0.2082 ns 0.0033 63 B
WriteAsyncChunked 93.8709 ns 1.3749 ns 0.0034 63 B
WriteAsyncAwaited 126.9702 ns 0.8035 ns 0.0034 63 B
WriteAsyncAwaitedChunked 129.1539 ns 2.0402 ns 0.0040 63 B
ProduceEnd 36.8412 ns 1.3556 ns 0.0085 63 B
ProduceEndChunked 35.2432 ns 0.2778 ns 0.0086 63 B
@halter73

This comment has been minimized.

Show comment
Hide comment
@halter73

halter73 Dec 10, 2016

Member

:shipit: *crosses fingers*

Member

halter73 commented Dec 10, 2016

:shipit: *crosses fingers*

@cesarbs cesarbs merged commit cedbe76 into dev Dec 13, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@cesarbs cesarbs deleted the cesarbs/client-fin-request-abort branch Dec 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment