Don't Leak Internal (server) Connections on Shutdown (#82)

This fixes an existing bug where a connection can be leaked if the library is cleaned up while there are any outstanding internal connections. The fix is to have the worker shut them down if there is no external owner when the worker's thread is stopped. The PR also disabled event validation tests on schannel, since they require the connection to succeed (which doesn't work on Azure Pipelines, because Schannel TLS 1.3 isn't supported yet).
dotnet · Jan 31, 2020 · 8d16374 · 8d16374
1 parent ddf22d5
commit 8d16374
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 8 deletions.
diff --git a/azure-pipelines.yml b/azure-pipelines.yml
@@ -47,7 +47,7 @@ strategy:
       platform: 'windows'
       config: 'schannel'
       buildArgs: '-Tls schannel'
-      testCmd: '.\.azure\test_windows.cmd ParameterValidation.*'
+      testCmd: '.\.azure\test_windows.cmd ParameterValidation.*:-*Events'
 
 pool:
   vmImage: '$(platform)-latest'

diff --git a/core/connection.c b/core/connection.c
@@ -43,12 +43,6 @@ QuicConnInitializeCrypto(
     _In_ QUIC_CONNECTION* Connection
     );
 
-_IRQL_requires_max_(PASSIVE_LEVEL)
-void
-QuicConnOnShutdownComplete(
-    _In_ QUIC_CONNECTION* Connection
-    );
-
 _IRQL_requires_max_(DISPATCH_LEVEL)
 __drv_allocatesMem(Mem)
 _Must_inspect_result_

diff --git a/core/connection.h b/core/connection.h
@@ -802,6 +802,12 @@ QuicConnCloseHandle(
     _In_ QUIC_CONNECTION* Connection
     );
 
+_IRQL_requires_max_(PASSIVE_LEVEL)
+void
+QuicConnOnShutdownComplete(
+    _In_ QUIC_CONNECTION* Connection
+    );
+
 #if QUIC_TEST_MODE
 _IRQL_requires_max_(DISPATCH_LEVEL)
 inline

diff --git a/core/worker.c b/core/worker.c
@@ -581,6 +581,14 @@ QUIC_THREAD_CALLBACK(QuicWorkerThread, Context)
         QUIC_CONNECTION* Connection =
             QUIC_CONTAINING_RECORD(
                 QuicListRemoveHead(&Worker->Connections), QUIC_CONNECTION, WorkerLink);
+        if (!Connection->State.ExternalOwner) {
+            //
+            // If there is no external owner, shut down the connection so that
+            // it's not leaked.
+            //
+            QuicTraceLogConnVerbose(AbandonOnLibShutdown, Connection, "Abandoning on shutdown");
+            QuicConnOnShutdownComplete(Connection);
+        }
         QuicConnRelease(Connection, QUIC_CONN_REF_WORKER);
     }
 
@@ -705,4 +713,4 @@ QuicWorkerPoolGetLeastLoadedWorker(
 
     WorkerPool->LastWorker = MinQueueDelayWorker;
     return MinQueueDelayWorker;
-}
+}