From 0293e53e11e3a3c09a95f106d7426ab405a81fdf Mon Sep 17 00:00:00 2001
From: David Fowler <davidfowl@gmail.com>
Date: Fri, 30 Mar 2018 15:51:48 -0700
Subject: [PATCH] Handle uninitialized connections in disposal (#1786) (#1794)

- We made a change to not initialize pipes up front
on connection creation. That change make it null ref in disposal because we didn't check if the pipes were initialized.
- Added a test
- Also fixed the EchoConnectionHandler in the functional ts tests.
---
 .../FunctionalTests/EchoConnectionHandler.cs  | 42 ++++++++-----------
 .../HttpConnectionContext.cs                  | 12 +++---
 .../HttpConnectionManagerTests.cs             | 14 +++++++
 3 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/clients/ts/FunctionalTests/EchoConnectionHandler.cs b/clients/ts/FunctionalTests/EchoConnectionHandler.cs
index 43ceb72e16..646d328a5b 100644
--- a/clients/ts/FunctionalTests/EchoConnectionHandler.cs
+++ b/clients/ts/FunctionalTests/EchoConnectionHandler.cs
@@ -11,35 +11,27 @@ namespace FunctionalTests
     {
         public async override Task OnConnectedAsync(ConnectionContext connection)
         {
-            var result = await connection.Transport.Input.ReadAsync();
-            var buffer = result.Buffer;
-
-            try
+            while (true)
             {
-                if (!buffer.IsEmpty)
+                var result = await connection.Transport.Input.ReadAsync();
+                var buffer = result.Buffer;
+
+                try
                 {
-                    await connection.Transport.Output.WriteAsync(buffer.ToArray());
+                    if (!buffer.IsEmpty)
+                    {
+                        await connection.Transport.Output.WriteAsync(buffer.ToArray());
+                    }
+                    else if (result.IsCompleted)
+                    {
+                        break;
+                    }
+                }
+                finally
+                {
+                    connection.Transport.Input.AdvanceTo(result.Buffer.End);
                 }
             }
-            finally
-            {
-                connection.Transport.Input.AdvanceTo(result.Buffer.End);
-            }
-
-            // Wait for the user to close
-            var tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
-            connection.Transport.Input.OnWriterCompleted((ex, state) => 
-            {
-                if (ex != null) 
-                {
-                    ((TaskCompletionSource<object>)state).TrySetException(ex);
-                }
-                else
-                {
-                    ((TaskCompletionSource<object>)state).TrySetResult(null);
-                }
-            }, tcs);
-            await tcs.Task;
         }
     }
 }
diff --git a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs
index c406dae6f7..bcbd63bf1c 100644
--- a/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs
+++ b/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionContext.cs
@@ -143,21 +143,21 @@ namespace Microsoft.AspNetCore.Http.Connections
                     // If the application task is faulted, propagate the error to the transport
                     if (ApplicationTask?.IsFaulted == true)
                     {
-                        Transport.Output.Complete(ApplicationTask.Exception.InnerException);
+                        Transport?.Output.Complete(ApplicationTask.Exception.InnerException);
                     }
                     else
                     {
-                        Transport.Output.Complete();
+                        Transport?.Output.Complete();
                     }
 
                     // If the transport task is faulted, propagate the error to the application
                     if (TransportTask?.IsFaulted == true)
                     {
-                        Application.Output.Complete(TransportTask.Exception.InnerException);
+                        Application?.Output.Complete(TransportTask.Exception.InnerException);
                     }
                     else
                     {
-                        Application.Output.Complete();
+                        Application?.Output.Complete();
                     }
 
                     var applicationTask = ApplicationTask ?? Task.CompletedTask;
@@ -180,8 +180,8 @@ namespace Microsoft.AspNetCore.Http.Connections
                 // REVIEW: Should we move this to the read loops?
 
                 // Complete the reading side of the pipes
-                Application.Input.Complete();
-                Transport.Input.Complete();
+                Application?.Input.Complete();
+                Transport?.Input.Complete();
             }
         }
 
diff --git a/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionManagerTests.cs b/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionManagerTests.cs
index 72ae778f84..33b6cdc0c3 100644
--- a/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionManagerTests.cs
+++ b/test/Microsoft.AspNetCore.Http.Connections.Tests/HttpConnectionManagerTests.cs
@@ -190,6 +190,20 @@ namespace Microsoft.AspNetCore.Http.Connections.Tests
             Assert.Equal(HttpConnectionContext.ConnectionStatus.Disposed, connection.Status);
         }
 
+        [Fact]
+        public async Task DisposeInactiveConnectionWithNoPipes()
+        {
+            var connectionManager = CreateConnectionManager();
+            var connection = connectionManager.CreateConnection();
+
+            Assert.NotNull(connection.ConnectionId);
+            Assert.Null(connection.Transport);
+            Assert.Null(connection.Application);
+
+            await connection.DisposeAsync();
+            Assert.Equal(HttpConnectionContext.ConnectionStatus.Disposed, connection.Status);
+        }
+
         [Fact]
         public void ScanAfterDisposeNoops()
         {