Make all UvRequest objects normal GC handles (#1698)

* Make all UvRequest objects normal GC handles
- This avoids the cost of using GCHandle.Alloc per operation.
- It *does* mean that we need to explicitly dispose UvRequest objects
after using them (which we did before anyways). This change does
add a few try catch statements to make sure we always dispose the UvRequest
if there are synchronous exceptions.
- This is ~1.5% of the overhead in the benchmarks today
- Keep track of all allocated UvRequest objects with a WeakReference in DEBUG
and assert none are kept around after cleaning up.
- Fixed a leak where we don't clean up UvWriteReq objects when writing
to the named pipe.

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/LibuvOutputConsumer.cs

@@ -47,14 +47,22 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
                     if (!buffer.IsEmpty)
                     {
                         var writeReq = _writeReqPool.Allocate();
-                        var writeResult = await writeReq.WriteAsync(_socket, buffer);
-                        _writeReqPool.Return(writeReq);
 
-                        LogWriteInfo(writeResult.Status, writeResult.Error);
+                        try
-
-                        if (writeResult.Error != null)
                         {
-                            throw writeResult.Error;
+                            var writeResult = await writeReq.WriteAsync(_socket, buffer);
+
+                            LogWriteInfo(writeResult.Status, writeResult.Error);
+
+                            if (writeResult.Error != null)
+                            {
+                                throw writeResult.Error;
+                            }
+                        }
+                        finally
+                        {
+                            // Make sure we return the writeReq to the pool
+                            _writeReqPool.Return(writeReq);
                         }
                     }
 
@@ -104,15 +112,23 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
             _log.ConnectionWriteFin(_connectionId);
 
             var shutdownReq = new UvShutdownReq(_log);
-            shutdownReq.Init(_thread.Loop);
+            try
-            shutdownReq.Shutdown(_socket, (req, status, state) =>
             {
-                req.Dispose();
+                shutdownReq.Init(_thread);
-                _log.ConnectionWroteFin(_connectionId, status);
+                shutdownReq.Shutdown(_socket, (req, status, state) =>
+                {
+                    req.Dispose();
+                    _log.ConnectionWroteFin(_connectionId, status);
 
-                tcs.TrySetResult(null);
+                    tcs.TrySetResult(null);
-            },
+                },
-            this);
+                this);
+            }
+            catch (Exception)
+            {
+                shutdownReq.Dispose();
+                throw;
+            }
 
             return tcs.Task;
         }

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/LibuvThread.cs

@@ -3,13 +3,14 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Runtime.ExceptionServices;
 using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Hosting;
-using Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking;
 using Microsoft.AspNetCore.Server.Kestrel.Internal.System.IO.Pipelines;
+using Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking;
 using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
@@ -76,6 +77,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
 
         public WriteReqPool WriteReqPool { get; }
 
+#if DEBUG
+        public List<WeakReference> Requests { get; } = new List<WeakReference>();
+#endif
+
         public ExceptionDispatchInfo FatalError { get { return _closeError; } }
 
         public Action<Action<IntPtr>, IntPtr> QueueCloseHandle { get; }
@@ -141,6 +146,21 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
             }
         }
 
+#if DEBUG
+        private void CheckUvReqLeaks()
+        {
+            GC.Collect();
+            GC.WaitForPendingFinalizers();
+            GC.Collect();
+
+            // Detect leaks in UvRequest objects
+            foreach (var request in Requests)
+            {
+                Debug.Assert(request.Target == null, $"{request.Target?.GetType()} object is still alive.");
+            }
+        }
+#endif
+
         private async Task DisposeConnectionsAsync()
         {
             // Close and wait for all connections
@@ -299,6 +319,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
                 WriteReqPool.Dispose();
                 thisHandle.Free();
                 _threadTcs.SetResult(null);
+
+#if DEBUG
+                // Check for handle leaks after disposing everything
+                CheckUvReqLeaks();
+#endif
             }
         }
 

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/ListenerPrimary.cs

@@ -105,17 +105,25 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
                 DetachFromIOCP(socket);
                 var dispatchPipe = _dispatchPipes[index];
                 var write = new UvWriteReq(Log);
-                write.Init(Thread.Loop);
+                try
-                write.Write2(
+                {
-                    dispatchPipe,
+                    write.Init(Thread);
-                    _dummyMessage,
+                    write.Write2(
-                    socket,
+                        dispatchPipe,
-                    (write2, status, error, state) =>
+                        _dummyMessage,
-                    {
+                        socket,
-                        write2.Dispose();
+                        (write2, status, error, state) =>
-                        ((UvStreamHandle)state).Dispose();
+                        {
-                    },
+                            write2.Dispose();
-                    socket);
+                            ((UvStreamHandle)state).Dispose();
+                        },
+                        socket);
+                }
+                catch (UvException)
+                {
+                    write.Dispose();
+                    throw;
+                }
             }
         }
 
@@ -230,7 +238,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
 
                         if (correctMessage)
                         {
-                            _listener._dispatchPipes.Add((UvPipeHandle) dispatchPipe);
+                            _listener._dispatchPipes.Add((UvPipeHandle)dispatchPipe);
                             dispatchPipe.ReadStop();
                             _bufHandle.Free();
                         }

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/ListenerSecondary.cs

@@ -59,11 +59,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
 
         private void StartedCallback(TaskCompletionSource<int> tcs)
         {
+            var connect = new UvConnectRequest(Log);
             try
             {
                 DispatchPipe.Init(Thread.Loop, Thread.QueueCloseHandle, true);
-                var connect = new UvConnectRequest(Log);
+                connect.Init(Thread);
-                connect.Init(Thread.Loop);
                 connect.Connect(
                     DispatchPipe,
                     _pipeName,
@@ -73,6 +73,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
             catch (Exception ex)
             {
                 DispatchPipe.Dispose();
+                connect.Dispose();
                 tcs.SetException(ex);
             }
         }
@@ -101,7 +102,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
                     (handle, status2, state) => ((ListenerSecondary)state).ReadStartCallback(handle, status2),
                     this);
 
-               writeReq.Init(Thread.Loop);
+               writeReq.Init(Thread);
                var result = await writeReq.WriteAsync(
                     DispatchPipe,
                     new ArraySegment<ArraySegment<byte>>(new [] { new ArraySegment<byte>(_pipeMessage) }));
@@ -117,10 +118,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
             }
             catch (Exception ex)
             {
-                writeReq.Dispose();
                 DispatchPipe.Dispose();
                 tcs.SetException(ex);
             }
+            finally
+            {
+                writeReq.Dispose();
+            }
         }
 
         private void ReadStartCallback(UvStreamHandle handle, int status)

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/Networking/UvConnectRequest.cs

@@ -20,7 +20,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
         {
         }
 
-        public void Init(UvLoopHandle loop)
+        public override void Init(LibuvThread thread)
+        {
+            DangerousInit(thread.Loop);
+
+            base.Init(thread);
+        }
+
+        public void DangerousInit(UvLoopHandle loop)
         {
             var requestSize = loop.Libuv.req_size(LibuvFunctions.RequestType.CONNECT);
             CreateMemory(
@@ -38,14 +45,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
             _callback = callback;
             _state = state;
 
-            Pin();
             Libuv.pipe_connect(this, pipe, name, _uv_connect_cb);
         }
 
         private static void UvConnectCb(IntPtr ptr, int status)
         {
             var req = FromIntPtr<UvConnectRequest>(ptr);
-            req.Unpin();
 
             var callback = req._callback;
             req._callback = null;

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/Networking/UvMemory.cs

@@ -17,10 +17,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
         protected LibuvFunctions _uv;
         protected int _threadId;
         protected readonly ILibuvTrace _log;
+        private readonly GCHandleType _handleType;
 
-        protected UvMemory(ILibuvTrace logger) : base(IntPtr.Zero, true)
+        protected UvMemory(ILibuvTrace logger, GCHandleType handleType = GCHandleType.Weak) : base(IntPtr.Zero, true)
         {
             _log = logger;
+            _handleType = handleType;
         }
 
         public LibuvFunctions Libuv { get { return _uv; } }
@@ -51,7 +53,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
             ThreadId = threadId;
             
             handle = Marshal.AllocCoTaskMem(size);
-            *(IntPtr*)handle = GCHandle.ToIntPtr(GCHandle.Alloc(this, GCHandleType.Weak));
+            *(IntPtr*)handle = GCHandle.ToIntPtr(GCHandle.Alloc(this, _handleType));
         }
 
         unsafe protected static void DestroyMemory(IntPtr memory)

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/Networking/UvRequest.cs

@@ -8,28 +8,25 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
 {
     public class UvRequest : UvMemory
     {
-        private GCHandle _pin;
+        protected UvRequest(ILibuvTrace logger) : base(logger, GCHandleType.Normal)
-
-        protected UvRequest(ILibuvTrace logger) : base (logger)
         {
         }
 
+        public virtual void Init(LibuvThread thread)
+        {
+#if DEBUG
+            // Store weak handles to all UvRequest objects so we can do leak detection
+            // while running tests
+            thread.Requests.Add(new WeakReference(this));
+#endif
+        }
+
         protected override bool ReleaseHandle()
         {
             DestroyMemory(handle);
             handle = IntPtr.Zero;
             return true;
         }
-
-        public virtual void Pin()
-        {
-            _pin = GCHandle.Alloc(this, GCHandleType.Normal);
-        }
-
-        public virtual void Unpin()
-        {
-            _pin.Free();
-        }
     }
 }
 

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/Networking/UvShutdownReq.cs

@@ -19,26 +19,28 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
         {
         }
 
-        public void Init(UvLoopHandle loop)
+        public override void Init(LibuvThread thread)
         {
+            var loop = thread.Loop;
+
             CreateMemory(
                 loop.Libuv, 
                 loop.ThreadId,
                 loop.Libuv.req_size(LibuvFunctions.RequestType.SHUTDOWN));
+
+            base.Init(thread);
         }
 
         public void Shutdown(UvStreamHandle handle, Action<UvShutdownReq, int, object> callback, object state)
         {
             _callback = callback;
             _state = state;
-            Pin();
             _uv.shutdown(this, handle, _uv_shutdown_cb);
         }
 
         private static void UvShutdownCb(IntPtr ptr, int status)
         {
             var req = FromIntPtr<UvShutdownReq>(ptr);
-            req.Unpin();
             req._callback(req, status, req._state);
             req._callback = null;
             req._state = null;

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/Networking/UvWriteReq.cs

@@ -31,7 +31,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
         {
         }
 
-        public void Init(UvLoopHandle loop)
+        public override void Init(LibuvThread thread)
+        {
+            DangerousInit(thread.Loop);
+
+            base.Init(thread);
+        }
+
+        public void DangerousInit(UvLoopHandle loop)
         {
             var requestSize = loop.Libuv.req_size(LibuvFunctions.RequestType.WRITE);
             var bufferSize = Marshal.SizeOf<LibuvFunctions.uv_buf_t>() * BUFFER_COUNT;
@@ -62,9 +69,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
         {
             try
             {
-                // add GCHandle to keeps this SafeHandle alive while request processing
-                _pins.Add(GCHandle.Alloc(this, GCHandleType.Normal));
-
                 var nBuffers = 0;
                 if (buffer.IsSingleSpan)
                 {
@@ -157,9 +161,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networkin
         {
             try
             {
-                // add GCHandle to keeps this SafeHandle alive while request processing
-                _pins.Add(GCHandle.Alloc(this, GCHandleType.Normal));
-
                 var pBuffers = (LibuvFunctions.uv_buf_t*)_bufs;
                 var nBuffers = bufs.Count;
                 if (nBuffers > BUFFER_COUNT)

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/WriteReqPool.cs

@@ -37,7 +37,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal
             else
             {
                 req = new UvWriteReq(_log);
-                req.Init(_thread.Loop);
+                req.Init(_thread);
             }
 
             return req;

test/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests/ListenerPrimaryTests.cs

@@ -131,7 +131,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests
                 var connectReq = new UvConnectRequest(connectionTrace);
 
                 pipe.Init(libuvThreadPrimary.Loop, libuvThreadPrimary.QueueCloseHandle);
-                connectReq.Init(libuvThreadPrimary.Loop);
+                connectReq.Init(libuvThreadPrimary);
 
                 connectReq.Connect(
                     pipe,

test/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests/MultipleLoopTests.cs

@@ -64,7 +64,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests
                 }
 
                 var writeRequest = new UvWriteReq(_logger);
-                writeRequest.Init(loop);
+                writeRequest.DangerousInit(loop);
 
                 await writeRequest.WriteAsync(
                     serverConnectionPipe,
@@ -84,7 +84,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests
 
                 loop2.Init(_uv);
                 clientConnectionPipe.Init(loop2, (a, b) => { }, true);
-                connect.Init(loop2);
+                connect.DangerousInit(loop2);
                 connect.Connect(clientConnectionPipe, pipeName, (handle, status, error, state) =>
                 {
                     var buf = loop2.Libuv.buf_init(Marshal.AllocHGlobal(8192), 8192);
@@ -157,7 +157,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests
                 serverConnectionPipeAcceptedEvent.WaitOne();
 
                 var writeRequest = new UvWriteReq(_logger);
-                writeRequest.Init(loop);
+                writeRequest.DangerousInit(loop);
                 writeRequest.Write2(
                     serverConnectionPipe,
                     new ArraySegment<ArraySegment<byte>>(new ArraySegment<byte>[] { new ArraySegment<byte>(new byte[] { 1, 2, 3, 4 }) }),
@@ -182,7 +182,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests
 
                 loop2.Init(_uv);
                 clientConnectionPipe.Init(loop2, (a, b) => { }, true);
-                connect.Init(loop2);
+                connect.DangerousInit(loop2);
                 connect.Connect(clientConnectionPipe, pipeName, (handle, status, error, state) =>
                 {
                     connect.Dispose();

test/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests/NetworkingTests.cs

@@ -160,7 +160,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests
                             for (var x = 0; x < 2; x++)
                             {
                                 var req = new UvWriteReq(_logger);
-                                req.Init(loop);
+                                req.DangerousInit(loop);
                                 var block = ReadableBuffer.Create(new byte[] { 65, 66, 67, 68, 69 });
 
                                 await req.WriteAsync(