From f21cb128e828053b86e033173d5921f9bd89ee3b Mon Sep 17 00:00:00 2001 From: Cesar Blum Silveira Date: Thu, 18 Feb 2016 15:48:13 -0800 Subject: [PATCH] Handle 0-byte reads correctly (#520). --- .../Http/Connection.cs | 16 +++++-- .../Http/SocketInput.cs | 16 +++++++ .../ConnectionTests.cs | 46 +++++++++++++++++++ .../SocketOutputTests.cs | 32 +++---------- .../TestHelpers/MockLibuv.cs | 19 ++++++++ .../TestHelpers/MockSocket.cs | 21 +++++++++ 6 files changed, 122 insertions(+), 28 deletions(-) create mode 100644 test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs create mode 100644 test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockSocket.cs diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Http/Connection.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Http/Connection.cs index 3cc06e8521..c0de2ed3b3 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Http/Connection.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Http/Connection.cs @@ -16,9 +16,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http // Base32 encoding - in ascii sort order for easy text based sorting private static readonly string _encode32Chars = "0123456789ABCDEFGHIJKLMNOPQRSTUV"; - private static readonly Action _readCallback = + private static readonly Action _readCallback = (handle, status, state) => ReadCallback(handle, status, state); - private static readonly Func _allocCallback = + private static readonly Func _allocCallback = (handle, suggestedsize, state) => AllocCallback(handle, suggestedsize, state); // Seed the _lastConnectionId for this application instance with @@ -252,8 +252,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http private void OnRead(UvStreamHandle handle, int status) { + if (status == 0) + { + // A zero status does not indicate an error or connection end. It indicates + // there is no data to be read right now. + // See the note at http://docs.libuv.org/en/v1.x/stream.html#c.uv_read_cb. + // We need to clean up whatever was allocated by OnAlloc. + _rawSocketInput.IncomingDeferred(); + return; + } + var normalRead = status > 0; - var normalDone = status == 0 || status == Constants.ECONNRESET || status == Constants.EOF; + var normalDone = status == Constants.ECONNRESET || status == Constants.EOF; var errorDone = !(normalDone || normalRead); var readCount = normalRead ? status : 0; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs index d5803f0698..a534fe9261 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Diagnostics; using System.IO; using System.Runtime.CompilerServices; using System.Threading; @@ -117,6 +118,21 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http Complete(); } + public void IncomingDeferred() + { + Debug.Assert(_pinned != null); + + if (_pinned != null) + { + if (_pinned != _tail) + { + _memory.Return(_pinned); + } + + _pinned = null; + } + } + private void Complete() { var awaitableState = Interlocked.Exchange( diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs new file mode 100644 index 0000000000..203a807267 --- /dev/null +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs @@ -0,0 +1,46 @@ +using System.Threading; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Server.Kestrel; +using Microsoft.AspNetCore.Server.Kestrel.Http; +using Microsoft.AspNetCore.Server.Kestrel.Infrastructure; +using Microsoft.AspNetCore.Server.Kestrel.Networking; +using Microsoft.AspNetCore.Server.KestrelTests.TestHelpers; +using Xunit; + +namespace Microsoft.AspNetCore.Server.KestrelTests +{ + public class ConnectionTests + { + [Fact] + public void DoesNotEndConnectionOnZeroRead() + { + var mockLibuv = new MockLibuv(); + + using (var memory = new MemoryPool2()) + using (var engine = new KestrelEngine(mockLibuv, new TestServiceContext())) + { + engine.Start(count: 1); + + var trace = new TestKestrelTrace(); + var context = new ListenerContext(new TestServiceContext()) + { + FrameFactory = connectionContext => new Frame( + new DummyApplication(httpContext => TaskUtilities.CompletedTask), connectionContext), + Memory2 = memory, + ServerAddress = ServerAddress.FromUrl($"http://localhost:{TestServer.GetNextPort()}"), + Thread = engine.Threads[0] + }; + var socket = new MockSocket(mockLibuv, Thread.CurrentThread.ManagedThreadId, trace); + var connection = new Connection(context, socket); + connection.Start(); + + Libuv.uv_buf_t ignored; + mockLibuv.AllocCallback(socket.InternalGetHandle(), 2048, out ignored); + mockLibuv.ReadCallback(socket.InternalGetHandle(), 0, ref ignored); + Assert.False(connection.SocketInput.RemoteIntakeFin); + + connection.ConnectionControl.End(ProduceEndType.SocketDisconnect); + } + } + } +} diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/SocketOutputTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/SocketOutputTests.cs index 8fa09e4866..f0132e0c82 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/SocketOutputTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/SocketOutputTests.cs @@ -39,7 +39,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests kestrelEngine.Start(count: 1); var kestrelThread = kestrelEngine.Threads[0]; - var socket = new MockSocket(kestrelThread.Loop.ThreadId, new TestKestrelTrace()); + var socket = new MockSocket(mockLibuv, kestrelThread.Loop.ThreadId, new TestKestrelTrace()); var trace = new KestrelTrace(new TestKestrelTrace()); var ltp = new LoggingThreadPool(trace); var socketOutput = new SocketOutput(kestrelThread, socket, memory, new MockConnection(), "0", trace, ltp, new Queue()); @@ -90,7 +90,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests kestrelEngine.Start(count: 1); var kestrelThread = kestrelEngine.Threads[0]; - var socket = new MockSocket(kestrelThread.Loop.ThreadId, new TestKestrelTrace()); + var socket = new MockSocket(mockLibuv, kestrelThread.Loop.ThreadId, new TestKestrelTrace()); var trace = new KestrelTrace(new TestKestrelTrace()); var ltp = new LoggingThreadPool(trace); var socketOutput = new SocketOutput(kestrelThread, socket, memory, new MockConnection(), "0", trace, ltp, new Queue()); @@ -158,7 +158,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests kestrelEngine.Start(count: 1); var kestrelThread = kestrelEngine.Threads[0]; - var socket = new MockSocket(kestrelThread.Loop.ThreadId, new TestKestrelTrace()); + var socket = new MockSocket(mockLibuv, kestrelThread.Loop.ThreadId, new TestKestrelTrace()); var trace = new KestrelTrace(new TestKestrelTrace()); var ltp = new LoggingThreadPool(trace); var socketOutput = new SocketOutput(kestrelThread, socket, memory, new MockConnection(), "0", trace, ltp, new Queue()); @@ -231,7 +231,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests kestrelEngine.Start(count: 1); var kestrelThread = kestrelEngine.Threads[0]; - var socket = new MockSocket(kestrelThread.Loop.ThreadId, new TestKestrelTrace()); + var socket = new MockSocket(mockLibuv, kestrelThread.Loop.ThreadId, new TestKestrelTrace()); var trace = new KestrelTrace(new TestKestrelTrace()); var ltp = new LoggingThreadPool(trace); @@ -341,7 +341,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests kestrelEngine.Start(count: 1); var kestrelThread = kestrelEngine.Threads[0]; - var socket = new MockSocket(kestrelThread.Loop.ThreadId, new TestKestrelTrace()); + var socket = new MockSocket(mockLibuv, kestrelThread.Loop.ThreadId, new TestKestrelTrace()); var trace = new KestrelTrace(new TestKestrelTrace()); var ltp = new LoggingThreadPool(trace); @@ -429,7 +429,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests kestrelEngine.Start(count: 1); var kestrelThread = kestrelEngine.Threads[0]; - var socket = new MockSocket(kestrelThread.Loop.ThreadId, new TestKestrelTrace()); + var socket = new MockSocket(mockLibuv, kestrelThread.Loop.ThreadId, new TestKestrelTrace()); var trace = new KestrelTrace(new TestKestrelTrace()); var ltp = new LoggingThreadPool(trace); var socketOutput = new SocketOutput(kestrelThread, socket, memory, new MockConnection(), "0", trace, ltp, new Queue()); @@ -515,7 +515,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests kestrelEngine.Start(count: 1); var kestrelThread = kestrelEngine.Threads[0]; - var socket = new MockSocket(kestrelThread.Loop.ThreadId, new TestKestrelTrace()); + var socket = new MockSocket(mockLibuv, kestrelThread.Loop.ThreadId, new TestKestrelTrace()); var trace = new KestrelTrace(new TestKestrelTrace()); var ltp = new LoggingThreadPool(trace); var socketOutput = new SocketOutput(kestrelThread, socket, memory, new MockConnection(), "0", trace, ltp, new Queue()); @@ -544,23 +544,5 @@ namespace Microsoft.AspNetCore.Server.KestrelTests default(ArraySegment), default(CancellationToken), socketDisconnect: true); } } - - - private class MockSocket : UvStreamHandle - { - public MockSocket(int threadId, IKestrelTrace logger) : base(logger) - { - // Set the handle to something other than IntPtr.Zero - // so handle.Validate doesn't fail in Libuv.write - handle = (IntPtr)1; - _threadId = threadId; - } - - protected override bool ReleaseHandle() - { - // No-op - return true; - } - } } } diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockLibuv.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockLibuv.cs index 2f2f2f1103..745a3eca11 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockLibuv.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockLibuv.cs @@ -17,6 +17,12 @@ namespace Microsoft.AspNetCore.Server.KestrelTests.TestHelpers unsafe public MockLibuv() : base(onlyForTesting: true) { + _onWrite = (socket, buffers, triggerCompleted) => + { + triggerCompleted(0); + return 0; + }; + _uv_write = UvWrite; _uv_async_send = postHandle => @@ -69,6 +75,8 @@ namespace Microsoft.AspNetCore.Server.KestrelTests.TestHelpers _uv_walk = (loop, callback, ignore) => 0; _uv_err_name = errno => IntPtr.Zero; _uv_strerror = errno => IntPtr.Zero; + _uv_read_start = UvReadStart; + _uv_read_stop = handle => 0; } public Func, int> OnWrite @@ -83,6 +91,17 @@ namespace Microsoft.AspNetCore.Server.KestrelTests.TestHelpers } } + public uv_alloc_cb AllocCallback { get; set; } + + public uv_read_cb ReadCallback { get; set; } + + private int UvReadStart(UvStreamHandle handle, uv_alloc_cb allocCallback, uv_read_cb readCallback) + { + AllocCallback = allocCallback; + ReadCallback = readCallback; + return 0; + } + unsafe private int UvWrite(UvRequest req, UvStreamHandle handle, uv_buf_t* bufs, int nbufs, uv_write_cb cb) { return _onWrite(handle, nbufs, status => cb(req.InternalGetHandle(), status)); diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockSocket.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockSocket.cs new file mode 100644 index 0000000000..4e41590dd9 --- /dev/null +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockSocket.cs @@ -0,0 +1,21 @@ +using System; +using Microsoft.AspNetCore.Server.Kestrel.Infrastructure; +using Microsoft.AspNetCore.Server.Kestrel.Networking; + +namespace Microsoft.AspNetCore.Server.KestrelTests.TestHelpers +{ + class MockSocket : UvStreamHandle + { + public MockSocket(Libuv uv, int threadId, IKestrelTrace logger) : base(logger) + { + CreateMemory(uv, threadId, IntPtr.Size); + } + + protected override bool ReleaseHandle() + { + DestroyMemory(handle); + handle = IntPtr.Zero; + return true; + } + } +}