Clean up libuv connection #1726
Conversation
| Input.Complete(); | ||
| } | ||
| } | ||
| _socket.ReadStart(_allocCallback, _readCallback, this); |
There was a problem hiding this comment.
I can add the try catch back but I'd like to understand if it's truly needed (and write a test for it maybe).
There was a problem hiding this comment.
I suspect this could fail for the same reason that uv_tcp_getpeername sometimes does on linux. This is normally caught and logged by the listener instead of crashing the server, but that wouldn't be the case for Resume().
Remember that before Resume() is called there is no read callback wired to tell us the underlying connection is no closed or invalid.
| Input.Complete(); | ||
| } | ||
| } | ||
| _socket.ReadStart(_allocCallback, _readCallback, this); |
There was a problem hiding this comment.
I suspect this could fail for the same reason that uv_tcp_getpeername sometimes does on linux. This is normally caught and logged by the listener instead of crashing the server, but that wouldn't be the case for Resume().
Remember that before Resume() is called there is no read callback wired to tell us the underlying connection is no closed or invalid.
| _socket.Dispose(); | ||
| // Make sure it isn't possible for a paused read to resume reading after calling uv_close | ||
| // on the stream handle | ||
| Input.CancelPendingFlush(); |
There was a problem hiding this comment.
Is there a Pipe test that verifies that the writer in fact observes cancellation when:
- The writer continuation is scheduled after a call to IPipeReader.Advance.
- Next CancelPendingFlush is called.
- Lastly the _writerScheduler actually runs the continuation.
It looks like this is already the case, but it's important this never regresses.
| } | ||
|
|
||
| [Fact] | ||
| public async Task ConnectionDoesNotResumeAfterSocketCloseIfBackpressureIsApplied() |
There was a problem hiding this comment.
Does this test actually consume the "request" data in such a way that the old code would have even tried to resume? Looking at the test, it seems like it would pass in dev since (await flushTask).IsCompleted would already be true.
There was a problem hiding this comment.
(await flushTask).IsCompleted is never true be cause we never complete the Input.Reader. Regardless, I'm attempting to add another test.
| var transportContext = new TestLibuvTransportContext() { ConnectionHandler = mockConnectionHandler }; | ||
| var transport = new LibuvTransport(mockLibuv, transportContext, null); | ||
| var thread = new LibuvThread(transport); | ||
| // We don't set the output scheduler here since we want to run the callback inline |
There was a problem hiding this comment.
Nit: We don't set the output writer scheduler
davidfowl
force-pushed
the
davidfowl/libuv-connection
branch
from
6055194 to
32dafb0
Compare
|
🆙 📅 |
- Cancel all pending flushes on the input writer before disposing the stream handle. - Complete the pipe before disposing the socket - Added logging for connection pause/resume. - Added test
davidfowl
force-pushed
the
davidfowl/libuv-connection
branch
from
32dafb0 to
99403b6
Compare
| // ReadStart() can throw a UvException in some cases (e.g. socket is no longer connected). | ||
| // This should be treated the same as OnRead() seeing a "normalDone" condition. | ||
| Log.ConnectionReadFin(ConnectionId); | ||
| Input.Complete(); |
There was a problem hiding this comment.
Ironically, this is the only place the input writer is completed without an error. You could fix #1331 right now by calling Input.Complete(uvEx) here.
| @@ -132,7 +132,7 @@ private void OnConnection(UvStreamHandle listenSocket, int status) | |||
| protected virtual void DispatchConnection(UvStreamHandle socket) | |||
| { | |||
| var connection = new LibuvConnection(this, socket); | |||
| connection.Start(); | |||
| _ = connection.Start(); | |||
There was a problem hiding this comment.
Soo much better than async void lol.
disposing the stream handle.