From bac0f44fa7e102cf8dc6112c7dfd9f227d56708a Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Fri, 21 Sep 2018 09:13:39 -0700 Subject: [PATCH] Revert "Disconnect the disconnect handler when request processing ends (#1413)" This reverts commit f808bdc331ed9e5555c315d44bcc9559695f041c. --- .../AspNetCore/DisconnectHandler.cpp | 15 ++--- .../AspNetCore/DisconnectHandler.h | 12 ++-- .../AspNetCore/proxymodule.cpp | 29 +++------ .../AspNetCore/proxymodule.h | 4 +- .../CommonLib/SRWExclusiveLock.cpp | 2 +- .../CommonLib/SRWExclusiveLock.h | 2 +- .../CommonLib/irequesthandler.h | 6 -- .../ClientDisconnectStress.cs | 65 ------------------- test/WebSites/InProcessWebSite/Startup.cs | 11 ---- 9 files changed, 20 insertions(+), 126 deletions(-) delete mode 100644 test/Common.FunctionalTests/ClientDisconnectStress.cs diff --git a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp index 3e8e7754aa..b921994850 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp @@ -4,21 +4,15 @@ #include "DisconnectHandler.h" #include "exceptions.h" #include "proxymodule.h" -#include "SRWExclusiveLock.h" void DisconnectHandler::NotifyDisconnect() { try { - std::unique_ptr module; - { - SRWExclusiveLock lock(m_handlerLock); - m_pHandler.swap(module); - } - + const auto module = m_pModule.exchange(nullptr); if (module != nullptr) { - module->NotifyDisconnect(); + module ->NotifyDisconnect(); } } catch (...) @@ -33,8 +27,7 @@ void DisconnectHandler::CleanupStoredContext() noexcept delete this; } -void DisconnectHandler::SetHandler(std::unique_ptr handler) noexcept +void DisconnectHandler::SetHandler(ASPNET_CORE_PROXY_MODULE * module) noexcept { - SRWExclusiveLock lock(m_handlerLock); - handler.swap(m_pHandler); + m_pModule = module; } diff --git a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h index 63b7525356..34c8a24a63 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h +++ b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h @@ -2,9 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. #pragma once - -#include -#include "irequesthandler.h" +#include class ASPNET_CORE_PROXY_MODULE; @@ -12,9 +10,8 @@ class DisconnectHandler final: public IHttpConnectionStoredContext { public: DisconnectHandler() - : m_pHandler(nullptr) + : m_pModule(nullptr) { - InitializeSRWLock(&m_handlerLock); } virtual @@ -30,10 +27,9 @@ public: CleanupStoredContext() noexcept override; void - SetHandler(std::unique_ptr handler) noexcept; + SetHandler(ASPNET_CORE_PROXY_MODULE * module) noexcept; private: - SRWLOCK m_handlerLock {}; - std::unique_ptr m_pHandler; + std::atomic m_pModule; }; diff --git a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp index 6f879f2e35..d3d71e8b4b 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp @@ -71,10 +71,6 @@ ASPNET_CORE_PROXY_MODULE::ASPNET_CORE_PROXY_MODULE(HTTP_MODULE_ID moduleId, std: ASPNET_CORE_PROXY_MODULE::~ASPNET_CORE_PROXY_MODULE() { - // At this point m_pDisconnectHandler should be disconnected in - // HandleNotificationStatus - assert(m_pDisconnectHandler == nullptr); - if (m_pDisconnectHandler != nullptr) { m_pDisconnectHandler->SetHandler(nullptr); @@ -116,6 +112,8 @@ ASPNET_CORE_PROXY_MODULE::OnExecuteRequestHandler( FINISHED_IF_FAILED(moduleContainer->SetConnectionModuleContext(static_cast(disconnectHandler.release()), m_moduleId)); } + m_pDisconnectHandler->SetHandler(this); + FINISHED_IF_FAILED(m_pApplicationManager->GetOrCreateApplicationInfo( *pHttpContext, m_pApplicationInfo)); @@ -123,8 +121,6 @@ ASPNET_CORE_PROXY_MODULE::OnExecuteRequestHandler( FINISHED_IF_FAILED(m_pApplicationInfo->CreateHandler(*pHttpContext, m_pHandler)); retVal = m_pHandler->OnExecuteRequestHandler(); - - m_pDisconnectHandler->SetHandler(::ReferenceRequestHandler(m_pHandler.get())); } catch (...) { @@ -145,7 +141,7 @@ Finished: } } - return HandleNotificationStatus(retVal); + return retVal; } __override @@ -160,27 +156,18 @@ ASPNET_CORE_PROXY_MODULE::OnAsyncCompletion( { try { - return HandleNotificationStatus(m_pHandler->OnAsyncCompletion( + return m_pHandler->OnAsyncCompletion( pCompletionInfo->GetCompletionBytes(), - pCompletionInfo->GetCompletionStatus())); + pCompletionInfo->GetCompletionStatus()); } catch (...) { OBSERVE_CAUGHT_EXCEPTION(); - return HandleNotificationStatus(RQ_NOTIFICATION_FINISH_REQUEST); + return RQ_NOTIFICATION_FINISH_REQUEST; } } -REQUEST_NOTIFICATION_STATUS ASPNET_CORE_PROXY_MODULE::HandleNotificationStatus(REQUEST_NOTIFICATION_STATUS status) noexcept +void ASPNET_CORE_PROXY_MODULE::NotifyDisconnect() const { - if (status != RQ_NOTIFICATION_PENDING) - { - if (m_pDisconnectHandler != nullptr) - { - m_pDisconnectHandler->SetHandler(nullptr); - m_pDisconnectHandler = nullptr; - } - } - - return status; + m_pHandler->NotifyDisconnect(); } diff --git a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.h b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.h index 24f4794c68..97c126b0f9 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.h +++ b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.h @@ -47,8 +47,8 @@ class ASPNET_CORE_PROXY_MODULE : NonCopyable, public CHttpModule IHttpCompletionInfo * pCompletionInfo ) override; - REQUEST_NOTIFICATION_STATUS - HandleNotificationStatus(REQUEST_NOTIFICATION_STATUS status) noexcept; + void + NotifyDisconnect() const; private: std::shared_ptr m_pApplicationManager; diff --git a/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.cpp b/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.cpp index 896189c8a8..0bc46b8cf5 100644 --- a/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.cpp +++ b/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.cpp @@ -3,7 +3,7 @@ #include "SRWExclusiveLock.h" -SRWExclusiveLock::SRWExclusiveLock(const SRWLOCK& lock) noexcept +SRWExclusiveLock::SRWExclusiveLock(const SRWLOCK& lock) : m_lock(lock) { AcquireSRWLockExclusive(const_cast(&m_lock)); diff --git a/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h b/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h index 6c17e13111..7ff977f70d 100644 --- a/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h +++ b/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h @@ -8,7 +8,7 @@ class SRWExclusiveLock { public: - SRWExclusiveLock(const SRWLOCK& lock) noexcept; + SRWExclusiveLock(const SRWLOCK& lock); ~SRWExclusiveLock(); private: const SRWLOCK& m_lock; diff --git a/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h b/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h index c89411f8a0..8bd2076072 100644 --- a/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h +++ b/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h @@ -54,9 +54,3 @@ struct IREQUEST_HANDLER_DELETER application->DereferenceRequestHandler(); } }; - -inline std::unique_ptr ReferenceRequestHandler(IREQUEST_HANDLER* handler) -{ - handler->ReferenceRequestHandler(); - return std::unique_ptr(handler); -}; diff --git a/test/Common.FunctionalTests/ClientDisconnectStress.cs b/test/Common.FunctionalTests/ClientDisconnectStress.cs deleted file mode 100644 index 9deeae3f92..0000000000 --- a/test/Common.FunctionalTests/ClientDisconnectStress.cs +++ /dev/null @@ -1,65 +0,0 @@ -// 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.Net.Http; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Server.IntegrationTesting; -using Microsoft.AspNetCore.Testing.xunit; -using Xunit; - -namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests -{ - [Collection(PublishedSitesCollection.Name)] - public class ClientDisconnectStressTests: FunctionalTestsBase - { - private readonly PublishedSitesFixture _fixture; - - public ClientDisconnectStressTests(PublishedSitesFixture fixture) - { - _fixture = fixture; - } - - [ConditionalTheory] - [InlineData(HostingModel.InProcess)] - [InlineData(HostingModel.OutOfProcess)] - public async Task ClientDisconnectStress(HostingModel hostingModel) - { - var site = await StartAsync(_fixture.GetBaseDeploymentParameters(hostingModel)); - var maxRequestSize = 1000; - var blockSize = 40; - var random = new Random(); - async Task RunRequests() - { - using (var connection = new TestConnection(site.HttpClient.BaseAddress.Port)) - { - await connection.Send( - "POST /ReadAndFlushEcho HTTP/1.1", - $"Content-Length: {maxRequestSize}", - "Host: localhost", - "Connection: close", - "", - ""); - - var disconnectAfter = random.Next(maxRequestSize); - var data = new byte[blockSize]; - for (int i = 0; i < disconnectAfter / blockSize; i++) - { - await connection.Stream.WriteAsync(data); - } - } - } - - List tasks = new List(); - for (int i = 0; i < 100; i++) - { - tasks.Add(Task.Run(RunRequests)); - } - - await Task.WhenAll(tasks); - - StopServer(); - } - } -} diff --git a/test/WebSites/InProcessWebSite/Startup.cs b/test/WebSites/InProcessWebSite/Startup.cs index eb84272b76..39d3b9f765 100644 --- a/test/WebSites/InProcessWebSite/Startup.cs +++ b/test/WebSites/InProcessWebSite/Startup.cs @@ -324,17 +324,6 @@ namespace TestSite result = await ctx.Request.Body.ReadAsync(readBuffer, 0, readBuffer.Length); } } - private async Task ReadAndFlushEcho(HttpContext ctx) - { - var readBuffer = new byte[4096]; - var result = await ctx.Request.Body.ReadAsync(readBuffer, 0, readBuffer.Length); - while (result != 0) - { - await ctx.Response.WriteAsync(Encoding.UTF8.GetString(readBuffer, 0, result)); - await ctx.Response.Body.FlushAsync(); - result = await ctx.Request.Body.ReadAsync(readBuffer, 0, readBuffer.Length); - } - } private async Task ReadAndWriteEchoLines(HttpContext ctx) {