Remove dependency on pipe events in HttpConnection #11132
Conversation
davidfowl
commented
Jun 12, 2019
davidfowl
commented
- Refactored the HttpConnection to not depend on OnReaderCompleted and OnWriterCompleted. Instead we use ConnectionClosed to detect the FIN that we propagate via ConnectedAborted.
- Fire ConnectionClosed when a FIN is received from both transports.
- Remove pipe completion from Http1Connection and Http1OutputProducer. Instead just return from request processing.
- Flow the CancellaitonToken property in RawStream
- Refactored the HttpConnection to not depend on OnReaderCompleted and OnWriterCompleted. Instead we use ConnectionClosed to detect the FIN that we propagate via ConnectedAborted. - Fire ConnectionClosed when a FIN is received from both transports. - Remove pipe completion from Http1Connection and Http1OutputProducer. Instead just return from request processing. - Flow the CancellaitonToken property in RawStream
src/Servers/Kestrel/Core/src/Adapter/Internal/AdaptedPipeline.cs
Outdated
Show resolved
Hide resolved
|
I'd also prefer to have this for #11109 |
There was a problem hiding this comment.
The change itself seems good, but I think we should go over this in person before merging. Also to figure out how we can sync this with https://github.com/aspnet/AspNetCore/pull/11109/checks?check_run_id=146487478. I'd rather merge this first before merging my PR as I don't want to have the pipe scheduler be thread pool.
src/Servers/Kestrel/Core/src/Adapter/Internal/AdaptedPipeline.cs
Outdated
Show resolved
Hide resolved
- Cancel the transport input on RawStream to yield the pending read. This is much more efficient than passing a cancellation token into everything (and more reliable) - Fixed the RequestTests to not depend on inline scheduling - Properly dispose things in the LibuvOutputConsumerTests
src/Servers/Kestrel/Core/src/Adapter/Internal/AdaptedPipeline.cs
Outdated
Show resolved
Hide resolved
| @@ -187,6 +173,10 @@ private async Task DoReceive() | |||
| { | |||
| // If Shutdown() has already bee called, assume that was the reason ProcessReceives() exited. | |||
| Input.Complete(_shutdownReason ?? error); | |||
|
|
|||
| FireConnectionClosed(); | |||
There was a problem hiding this comment.
This makes me wish we called to ConnectionClosed token FinReceived or something like that, because the connection might be only half closed at this point.
There was a problem hiding this comment.
You could argue the same about HttpContext.ConnectionAborted, I don't think it makes a difference.
There was a problem hiding this comment.
You absolutely could, but at least that's HTTP-specific so it makes a bit more sense.
| private void FireConnectionClosed() | ||
| { | ||
| // Guard against scheduling this multiple times | ||
| if (_connectionClosed) |
There was a problem hiding this comment.
I see you're taking advantage of the single-threaded nature of libuv's event loop 👍
| // Fire the connection closed token and wait for it to complete | ||
| var waitForConnectionClosedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
| // Ensure this always fires | ||
| FireConnectionClosed(); |
There was a problem hiding this comment.
If we're going to continue firing ConnectionClosed for libuv output errors, we should do the same for the socket transport.
There was a problem hiding this comment.
What do you mean? Aren't we doing that? It always fires no?
There was a problem hiding this comment.
I'm now thinking about making this more like the socket transport instead. The important thing is consistency.
Instead of calling FireConnectionClosed here, just don't dispose _socket here. Instead, close the uv_stream_handle only after both the output loop completed and
a) you've received a terminal read status or
b) the connection was aborted
In the case of b) you'd have to make sure to still call FireConnectionClosed when you close the socket ofc.
| @@ -856,6 +852,7 @@ await using (var server = new TestServer(TestApp.EchoAppChunked, testContext)) | |||
| await connection.Send( | |||
| "POST / HTTP/1.1"); | |||
| connection.ShutdownSend(); | |||
| await connection.TransportConnection.WaitForCloseTask; | |||
There was a problem hiding this comment.
Discussed this offline. Calling ReceiveEnd unblocks the ProcessRequest loop allowing to observe the FIN and write a 4XX before the transport is aborted.
| } | ||
|
|
||
| public override ValueTask<int> ReadAsync(Memory<byte> destination, CancellationToken cancellationToken = default) | ||
| { | ||
| return ReadAsyncInternal(destination); | ||
| return ReadAsyncInternal(destination, cancellationToken); |
There was a problem hiding this comment.
Now that we're flowing the cancellationToken through RawStream, do we have a regression test?
src/Servers/Kestrel/Transport.Libuv/src/Internal/LibuvConnection.cs
Outdated
Show resolved
Hide resolved
- Add CancelPendingRead to RawStream - Clean up adapted pipeline complete - Skipped flaky test
|
@aspnet/build What's the deal with all the PRs, do we merge, do we wait? I know it's being looked at but what is the mitigation to unblock work? |
|
|
||
| public void CancelPendingRead() | ||
| { | ||
| _cancelCalled = true; |
There was a problem hiding this comment.
Does CancelPendingRead need to be synchronized?
There was a problem hiding this comment.
I don't even think _cancelCalled needs to be volatile since CancelPendingRead acquires and releases the Pipe's _sync lock right afterwards.
|
@davidfowl as a few people have done, I suggest making a polite request for an admin to merge PRs where build success isn't being reported correctly or individual jobs hang. Looks like both of those issues have hit this PR but is it ready to merge? |
I can merge I just want to know the protocol. Yes this is ready to merge. |
|
🆗, ping if you're blocked |
ghost
deleted the
amcasey
added
area-networking
