HTTP/3: Response drain timeout #35322

JamesNK · 2021-08-13T03:34:01Z

src/Servers/Kestrel/Core/src/Internal/Infrastructure/ITimeoutControl.cs

src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs

halter73 · 2021-08-16T19:26:14Z

src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs

@@ -415,6 +426,11 @@ private void ShutdownWrite(Exception? shutdownReason)

        public override async ValueTask DisposeAsync()
        {
+            if (_stream == null)
+            {
+                return;


This scares me considering DisposeAsync() pools. Does this mean something could still have a reference to the QuicStreamContext and dispose it after pooling? Maybe we should move the pooling logic outside of DisposeAsync since this is exposed to connection middleware and DisposeAsync() can be called multiple times.

I added that when I was considering a design that wasn't as safe. I quickly decided that it wasn't a good idea. This was leftover from the first attempt. Do you want it removed?

When a QUIC stream is disposed of the callback removes it from the connection before it is pooled. And only gracefully completed requests are pooled. No active request will be using a pooled stream.

What happens if connection middleware disposes the ConnectionContext before exiting?

If the app (which is Kestrel HTTP/3 implementation in this case) hasn't completed the pipes then it isn't pooled.

aspnetcore/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs

Lines 418 to 422 in a079651

CanReuse = _stream.CanRead && _stream.CanWrite

&& _transportPipeReader.IsCompletedSuccessfully

&& _transportPipeWriter.IsCompletedSuccessfully

&& !_clientAbort

&& !_serverAborted;

But third-party middleware could complete the Pipes, right? Sorry to randomize this PR. I do not think the pooling logic needs to be reworked before merging this. And I'll admit this likely isn't the most likely scenario. It's just seeing this got me thinking about pooling too early, and if it did become a problem, it might be very hard to diagnose.

Yes.

There has to be some public API and condition at which point the context is pooled. Whatever we use, there is the ability for someone to use the public API to trigger pooling early.

JamesNK · 2021-08-19T09:06:40Z

/backport to release/6.0-rc1

github-actions · 2021-08-19T09:06:54Z

Started backporting to release/6.0-rc1: https://github.com/dotnet/aspnetcore/actions/runs/1146342601

github-actions · 2021-08-19T09:07:35Z

@JamesNK backporting to release/6.0-rc1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: HTTP/3 response drain timeout
Using index info to reconstruct a base tree...
M	src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs
M	src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs
CONFLICT (content): Merge conflict in src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs
Auto-merging src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs
CONFLICT (content): Merge conflict in src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 HTTP/3 response drain timeout
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

- backport of #35322 to release/6.0-rc1

* Update dependencies from https://github.com/dotnet/efcore build 20210821.6 (#35617) Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design From Version 6.0.0-rc.1.21420.45 -> To Version 6.0.0-rc.1.21421.6 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Update docker container to mcr (#35630) Co-authored-by: Brennan <[email protected]> * [release/6.0-rc1] Update dependencies from dotnet/runtime dotnet/efcore (#35634) [release/6.0-rc1] Update dependencies from dotnet/runtime dotnet/efcore * [release/6.0-rc1] HTTP/3: Response drain timeout (#35492) - backport of #35322 to release/6.0-rc1 * [release/6.0-rc1] Reliability improvement for input date E2E tests (#35616) * Reliability improvement for input date E2E tests * Avoid "collection was modified" error in CircuitGracefulTerminationTests * Avoid timing issues in CanFocusDuringOnAfterRenderAsyncWithFocusInEvent * Update src/Components/test/E2ETest/Tests/FormsTest.cs Co-authored-by: Tanay Parikh <[email protected]> * Remove notes from earlier Co-authored-by: Steve Sanderson <[email protected]> Co-authored-by: Tanay Parikh <[email protected]> * Minimal APIs naming cleanup * Add Support for `DateOnly` & `TimeOnly` for `SupplyParameterFromQuery` Fixes: #35525 API Proposal: #35567 * Add FailureReasons (#35425) * Add support for Results extension point Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Brennan <[email protected]> Co-authored-by: James Newton-King <[email protected]> Co-authored-by: Steve Sanderson <[email protected]> Co-authored-by: Tanay Parikh <[email protected]> Co-authored-by: Stephen Halter <[email protected]> Co-authored-by: Tanay Parikh <[email protected]> Co-authored-by: Hao Kung <[email protected]> Co-authored-by: Safia Abdalla <[email protected]> Co-authored-by: Doug Bunting <[email protected]>

JamesNK requested review from BrennanConroy, halter73 and Tratcher as code owners August 13, 2021 03:34

ghost added the area-runtime label Aug 13, 2021

JamesNK force-pushed the jamesnk/http3-responsedrain branch from b69ff34 to 6aba457 Compare August 13, 2021 08:06

Tratcher reviewed Aug 13, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/ITimeoutControl.cs Outdated Show resolved Hide resolved

src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs Show resolved Hide resolved

halter73 reviewed Aug 16, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs Outdated Show resolved Hide resolved

halter73 reviewed Aug 16, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs Outdated Show resolved Hide resolved

halter73 reviewed Aug 16, 2021

View reviewed changes

JamesNK force-pushed the jamesnk/http3-responsedrain branch from 6aba457 to d760ea4 Compare August 16, 2021 22:27

halter73 approved these changes Aug 18, 2021

View reviewed changes

JamesNK force-pushed the jamesnk/http3-responsedrain branch from 7d92975 to 915a99b Compare August 18, 2021 05:02

JamesNK enabled auto-merge (squash) August 18, 2021 05:04

JamesNK force-pushed the jamesnk/http3-responsedrain branch 2 times, most recently from 381711f to d7d880b Compare August 18, 2021 20:32

HTTP/3 response drain timeout

7557071

JamesNK force-pushed the jamesnk/http3-responsedrain branch from d7d880b to 7557071 Compare August 18, 2021 22:13

JamesNK merged commit ff9ed0f into main Aug 18, 2021

JamesNK deleted the jamesnk/http3-responsedrain branch August 18, 2021 23:42

ghost added this to the 7.0-preview1 milestone Aug 18, 2021

halter73 mentioned this pull request Aug 23, 2021

[release/6.0-rc1] HTTP/3: Response drain timeout #35492

Merged

dougbu pushed a commit that referenced this pull request Aug 24, 2021

[release/6.0-rc1] HTTP/3: Response drain timeout (#35492)

c3c9732

- backport of #35322 to release/6.0-rc1

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HTTP/3: Response drain timeout #35322

HTTP/3: Response drain timeout #35322

Uh oh!

JamesNK commented Aug 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 Aug 16, 2021

Uh oh!

JamesNK Aug 16, 2021 •

edited

Loading

Uh oh!

halter73 Aug 16, 2021

Uh oh!

JamesNK Aug 16, 2021

Uh oh!

halter73 Aug 16, 2021

Uh oh!

JamesNK Aug 16, 2021

Uh oh!

JamesNK commented Aug 19, 2021

Uh oh!

github-actions bot commented Aug 19, 2021

Uh oh!

github-actions bot commented Aug 19, 2021

Uh oh!

Uh oh!

	CanReuse = _stream.CanRead && _stream.CanWrite
	&& _transportPipeReader.IsCompletedSuccessfully
	&& _transportPipeWriter.IsCompletedSuccessfully
	&& !_clientAbort
	&& !_serverAborted;

HTTP/3: Response drain timeout #35322

HTTP/3: Response drain timeout #35322

Uh oh!

Conversation

JamesNK commented Aug 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

JamesNK Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

JamesNK Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

halter73 Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

JamesNK Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

JamesNK commented Aug 19, 2021

Uh oh!

github-actions bot commented Aug 19, 2021

Uh oh!

github-actions bot commented Aug 19, 2021

Uh oh!

Uh oh!

JamesNK Aug 16, 2021 •

edited

Loading