From 0e04527fb4f5005965332907cd6b265327f0e21e Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Thu, 11 Oct 2018 08:44:03 -0700 Subject: [PATCH] General cleanup for client disconnect handling. (#1446) --- .../AspNetCore/DisconnectHandler.cpp | 33 ++++++++++---- .../AspNetCore/DisconnectHandler.h | 12 ++++-- .../AspNetCore/proxymodule.cpp | 5 +-- .../inprocesshandler.cpp | 33 +++++++++++--- .../inprocesshandler.h | 2 + .../Core/IISHttpContext.IO.cs | 19 +++++--- .../Core/IISHttpContext.cs | 4 +- .../Core/IISHttpServer.cs | 1 - .../Inprocess/ClientDisconnectTests.cs | 43 +++++++++++++++++++ 9 files changed, 123 insertions(+), 29 deletions(-) diff --git a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp index 3e8e7754aa..ad2ff17b47 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp @@ -10,15 +10,16 @@ void DisconnectHandler::NotifyDisconnect() { try { - std::unique_ptr module; + std::unique_ptr pHandler; { SRWExclusiveLock lock(m_handlerLock); - m_pHandler.swap(module); + m_pHandler.swap(pHandler); + m_disconnectFired = true; } - if (module != nullptr) + if (pHandler != nullptr) { - module->NotifyDisconnect(); + pHandler->NotifyDisconnect(); } } catch (...) @@ -29,12 +30,28 @@ void DisconnectHandler::NotifyDisconnect() void DisconnectHandler::CleanupStoredContext() noexcept { - SetHandler(nullptr); delete this; } -void DisconnectHandler::SetHandler(std::unique_ptr handler) noexcept +void DisconnectHandler::SetHandler(std::unique_ptr handler) { - SRWExclusiveLock lock(m_handlerLock); - handler.swap(m_pHandler); + IREQUEST_HANDLER* pHandler = nullptr; + { + SRWExclusiveLock lock(m_handlerLock); + + handler.swap(m_pHandler); + pHandler = m_pHandler.get(); + } + + assert(pHandler != nullptr); + + if (pHandler != nullptr && (m_disconnectFired || m_pHttpConnection != nullptr && !m_pHttpConnection->IsConnected())) + { + pHandler->NotifyDisconnect(); + } +} + +void DisconnectHandler::RemoveHandler() noexcept +{ + m_pHandler = nullptr; } diff --git a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h index 63b7525356..36d404edd6 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h +++ b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h @@ -11,8 +11,8 @@ class ASPNET_CORE_PROXY_MODULE; class DisconnectHandler final: public IHttpConnectionStoredContext { public: - DisconnectHandler() - : m_pHandler(nullptr) + DisconnectHandler(IHttpConnection* pHttpConnection) + : m_pHandler(nullptr), m_pHttpConnection(pHttpConnection), m_disconnectFired(false) { InitializeSRWLock(&m_handlerLock); } @@ -20,7 +20,7 @@ public: virtual ~DisconnectHandler() { - SetHandler(nullptr); + RemoveHandler(); } void @@ -30,10 +30,14 @@ public: CleanupStoredContext() noexcept override; void - SetHandler(std::unique_ptr handler) noexcept; + SetHandler(std::unique_ptr handler); + + void RemoveHandler() noexcept; private: SRWLOCK m_handlerLock {}; std::unique_ptr m_pHandler; + IHttpConnection* m_pHttpConnection; + bool m_disconnectFired; }; diff --git a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp index 38eaf08c31..e6b234d667 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp @@ -96,7 +96,6 @@ ASPNET_CORE_PROXY_MODULE::OnExecuteRequestHandler( FINISHED(HRESULT_FROM_WIN32(ERROR_SERVER_SHUTDOWN_IN_PROGRESS)); } - FINISHED_IF_FAILED(m_pApplicationManager->GetOrCreateApplicationInfo( *pHttpContext, m_pApplicationInfo)); @@ -185,7 +184,7 @@ void ASPNET_CORE_PROXY_MODULE::SetupDisconnectHandler(IHttpContext * pHttpContex if (pDisconnectHandler == nullptr) { - auto newHandler = std::make_unique(); + auto newHandler = std::make_unique(pHttpContext->GetConnection()); pDisconnectHandler = newHandler.get(); // ModuleContextContainer takes ownership of disconnectHandler // we are trusting that it would not release it before deleting the context @@ -207,6 +206,6 @@ void ASPNET_CORE_PROXY_MODULE::RemoveDisconnectHandler() noexcept if (handler != nullptr) { - handler->SetHandler(nullptr); + handler->RemoveHandler(); } } diff --git a/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp b/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp index 75d555e053..16938d0009 100644 --- a/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp +++ b/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp @@ -26,8 +26,10 @@ IN_PROCESS_HANDLER::IN_PROCESS_HANDLER( m_pRequestHandlerContext(pRequestHandlerContext), m_pAsyncCompletionHandler(pAsyncCompletion), m_pDisconnectHandler(pDisconnectHandler), - m_moduleTracer(pW3Context->GetTraceContext()) + m_moduleTracer(pW3Context->GetTraceContext()), + m_disconnectFired(false) { + InitializeSRWLock(&m_srwDisconnectLock); } __override @@ -92,16 +94,29 @@ REQUEST_NOTIFICATION_STATUS IN_PROCESS_HANDLER::ServerShutdownMessage() VOID IN_PROCESS_HANDLER::NotifyDisconnect() { + m_moduleTracer.RequestDisconnect(); + if (m_pApplication->QueryBlockCallbacksIntoManaged() || m_fManagedRequestComplete) { return; } - m_moduleTracer.RequestDisconnect(); + // NotifyDisconnect can be called before the m_pManagedHttpContext is set, + // so save that in a bool. + // Don't lock when calling m_pDisconnect to avoid the potential deadlock between this + // and SetManagedHttpContext + void* pManagedHttpContext = nullptr; + { + SRWExclusiveLock lock(m_srwDisconnectLock); + pManagedHttpContext = m_pManagedHttpContext; + m_disconnectFired = true; + } - assert(m_pManagedHttpContext != nullptr); - m_pDisconnectHandler(m_pManagedHttpContext); + if (pManagedHttpContext != nullptr) + { + m_pDisconnectHandler(m_pManagedHttpContext); + } } VOID @@ -127,7 +142,15 @@ IN_PROCESS_HANDLER::SetManagedHttpContext( PVOID pManagedHttpContext ) { - m_pManagedHttpContext = pManagedHttpContext; + { + SRWExclusiveLock lock(m_srwDisconnectLock); + m_pManagedHttpContext = pManagedHttpContext; + } + + if (m_disconnectFired && m_pManagedHttpContext != nullptr) + { + m_pDisconnectHandler(m_pManagedHttpContext); + } } // static diff --git a/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.h b/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.h index 177c25e329..743c2323a1 100644 --- a/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.h +++ b/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.h @@ -85,4 +85,6 @@ private: PFN_DISCONNECT_HANDLER m_pDisconnectHandler; static ALLOC_CACHE_HANDLER * sm_pAlloc; ModuleTracer m_moduleTracer; + bool m_disconnectFired; + SRWLOCK m_srwDisconnectLock; }; diff --git a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.IO.cs b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.IO.cs index 09c205d8de..724d2e55e1 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.IO.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.IO.cs @@ -190,13 +190,20 @@ namespace Microsoft.AspNetCore.Server.IIS.Core _bodyOutput.Dispose(); - try + var cts = _abortedCts; + if (cts != null) { - _abortedCts?.Cancel(); - } - catch (Exception) - { - // ignore + ThreadPool.QueueUserWorkItem(t => + { + try + { + cts.Cancel(); + } + catch (Exception) + { + // ignore + } + }); } } diff --git a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs index b824c67c6c..01aeeebec6 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.cs @@ -109,8 +109,6 @@ namespace Microsoft.AspNetCore.Server.IIS.Core { _thisHandle = GCHandle.Alloc(this); - NativeMethods.HttpSetManagedContext(_pInProcessHandler, (IntPtr)_thisHandle); - Method = GetVerb(); RawTarget = GetRawUrl(); @@ -170,6 +168,8 @@ namespace Microsoft.AspNetCore.Server.IIS.Core resumeWriterThreshold: ResumeWriterTheshold, minimumSegmentSize: MinAllocBufferSize)); _bodyOutput = new OutputProducer(pipe); + + NativeMethods.HttpSetManagedContext(_pInProcessHandler, (IntPtr)_thisHandle); } private string GetOriginalPath() diff --git a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpServer.cs b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpServer.cs index 7e540ddd83..7cde852570 100644 --- a/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpServer.cs +++ b/src/Microsoft.AspNetCore.Server.IIS/Core/IISHttpServer.cs @@ -213,7 +213,6 @@ namespace Microsoft.AspNetCore.Server.IIS.Core { context?.Server._logger.LogError(0, ex, $"Unexpected exception in {nameof(IISHttpServer)}.{nameof(OnDisconnect)}."); } - } private static NativeMethods.REQUEST_NOTIFICATION_STATUS OnAsyncCompletion(IntPtr pvManagedHttpContext, int hr, int bytes) diff --git a/test/Common.FunctionalTests/Inprocess/ClientDisconnectTests.cs b/test/Common.FunctionalTests/Inprocess/ClientDisconnectTests.cs index 761c389d48..6e2d1bc752 100644 --- a/test/Common.FunctionalTests/Inprocess/ClientDisconnectTests.cs +++ b/test/Common.FunctionalTests/Inprocess/ClientDisconnectTests.cs @@ -1,8 +1,12 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Collections.Generic; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Testing.xunit; +using Microsoft.Extensions.Logging; using Xunit; namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests @@ -61,5 +65,44 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests await _fixture.Client.RetryRequestAsync("/WaitingRequestCount", async message => await message.Content.ReadAsStringAsync() == "0"); } + + [ConditionalFact] + public async Task ClientDisconnectCallbackStress() + { + // Fixture initialization fails if inside of the Task.Run, so send an + // initial request to initialize the fixture. + var response = await _fixture.Client.GetAsync("HelloWorld"); + var numTotalRequests = 0; + for (var j = 0; j < 20; j++) + { + // Windows has a max connection limit of 10 for the IIS server, + // so setting limit fairly low. + const int numRequests = 5; + async Task RunRequests() + { + using (var connection = _fixture.CreateTestConnection()) + { + await connection.Send( + "GET /WaitForAbort HTTP/1.1", + "Host: localhost", + "Connection: close", + "", + ""); + await _fixture.Client.RetryRequestAsync("/WaitingRequestCount", async message => await message.Content.ReadAsStringAsync() != "0"); + Interlocked.Increment(ref numTotalRequests); + } + } + + List tasks = new List(); + for (int i = 0; i < numRequests; i++) + { + tasks.Add(Task.Run(RunRequests)); + } + + await Task.WhenAll(tasks); + + await _fixture.Client.RetryRequestAsync("/WaitingRequestCount", async message => await message.Content.ReadAsStringAsync() == "0"); + } + } } }