From 14aebebbaaabc64cfd737e9d66225b735cf52276 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 21 Sep 2018 08:26:20 -0700 Subject: [PATCH 1/2] Remove using environment variables for specifying tests. (#1410) --- .../Inprocess/LogPipeTests.cs | 12 +++-- test/Common.FunctionalTests/LogFileTests.cs | 1 - test/WebSites/InProcessWebSite/Program.cs | 44 ++++++++++--------- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/test/Common.FunctionalTests/Inprocess/LogPipeTests.cs b/test/Common.FunctionalTests/Inprocess/LogPipeTests.cs index ed48204385..4d34c3154f 100644 --- a/test/Common.FunctionalTests/Inprocess/LogPipeTests.cs +++ b/test/Common.FunctionalTests/Inprocess/LogPipeTests.cs @@ -1,7 +1,6 @@ // 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.IO; using System.Net; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.IIS.FunctionalTests.Utilities; @@ -41,19 +40,19 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests } [ConditionalTheory] - [InlineData("ConsoleErrorWrite")] - [InlineData("ConsoleWrite")] + [InlineData("ConsoleErrorWriteStartServer")] + [InlineData("ConsoleWriteStartServer")] public async Task CheckStdoutLoggingToPipeWithFirstWrite(string path) { var deploymentParameters = _fixture.GetBaseDeploymentParameters(publish: true); - var firstWriteString = path + path; + var firstWriteString = "TEST MESSAGE"; - deploymentParameters.WebConfigBasedEnvironmentVariables["ASPNETCORE_INPROCESS_INITIAL_WRITE"] = firstWriteString; + deploymentParameters.TransformArguments((a, _) => $"{a} {path}"); var deploymentResult = await DeployAsync(deploymentParameters); - await Helpers.AssertStarts(deploymentResult, path); + await Helpers.AssertStarts(deploymentResult); StopServer(); @@ -61,7 +60,6 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests { // We can't read stdout logs from IIS as they aren't redirected. Assert.Contains(TestSink.Writes, context => context.Message.Contains(firstWriteString)); - Assert.Contains(TestSink.Writes, context => context.Message.Contains("TEST MESSAGE")); } } diff --git a/test/Common.FunctionalTests/LogFileTests.cs b/test/Common.FunctionalTests/LogFileTests.cs index be8d8e700c..17f9def2d7 100644 --- a/test/Common.FunctionalTests/LogFileTests.cs +++ b/test/Common.FunctionalTests/LogFileTests.cs @@ -3,7 +3,6 @@ using System; using System.IO; -using System.Net; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.IIS.FunctionalTests.Utilities; using Microsoft.AspNetCore.Server.IntegrationTesting; diff --git a/test/WebSites/InProcessWebSite/Program.cs b/test/WebSites/InProcessWebSite/Program.cs index f7abe535ae..43e9a563b2 100644 --- a/test/WebSites/InProcessWebSite/Program.cs +++ b/test/WebSites/InProcessWebSite/Program.cs @@ -79,31 +79,33 @@ namespace TestSite host.Run(); } break; + case "ConsoleErrorWriteStartServer": + Console.Error.WriteLine("TEST MESSAGE"); + return StartServer(); + case "ConsoleWriteStartServer": + Console.WriteLine("TEST MESSAGE"); + return StartServer(); default: - { + return StartServer(); - var envVariable = Environment.GetEnvironmentVariable("ASPNETCORE_INPROCESS_INITIAL_WRITE"); - if (!string.IsNullOrEmpty(envVariable)) - { - Console.WriteLine(envVariable); - Console.Error.WriteLine(envVariable); - } - - var host = new WebHostBuilder() - .ConfigureLogging((_, factory) => - { - factory.AddConsole(); - factory.AddFilter("Console", level => level >= LogLevel.Information); - }) - .UseIIS() - .UseStartup() - .Build(); - - host.Run(); - return 0; - } } return 12; } + + private static int StartServer() + { + var host = new WebHostBuilder() + .ConfigureLogging((_, factory) => + { + factory.AddConsole(); + factory.AddFilter("Console", level => level >= LogLevel.Information); + }) + .UseIIS() + .UseStartup() + .Build(); + + host.Run(); + return 0; + } } } From f808bdc331ed9e5555c315d44bcc9559695f041c Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Fri, 21 Sep 2018 09:13:39 -0700 Subject: [PATCH 2/2] Disconnect the disconnect handler when request processing ends (#1413) --- .../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, 126 insertions(+), 20 deletions(-) create mode 100644 test/Common.FunctionalTests/ClientDisconnectStress.cs diff --git a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp index b921994850..3e8e7754aa 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.cpp @@ -4,15 +4,21 @@ #include "DisconnectHandler.h" #include "exceptions.h" #include "proxymodule.h" +#include "SRWExclusiveLock.h" void DisconnectHandler::NotifyDisconnect() { try { - const auto module = m_pModule.exchange(nullptr); + std::unique_ptr module; + { + SRWExclusiveLock lock(m_handlerLock); + m_pHandler.swap(module); + } + if (module != nullptr) { - module ->NotifyDisconnect(); + module->NotifyDisconnect(); } } catch (...) @@ -27,7 +33,8 @@ void DisconnectHandler::CleanupStoredContext() noexcept delete this; } -void DisconnectHandler::SetHandler(ASPNET_CORE_PROXY_MODULE * module) noexcept +void DisconnectHandler::SetHandler(std::unique_ptr handler) noexcept { - m_pModule = module; + SRWExclusiveLock lock(m_handlerLock); + handler.swap(m_pHandler); } diff --git a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h index 34c8a24a63..63b7525356 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h +++ b/src/AspNetCoreModuleV2/AspNetCore/DisconnectHandler.h @@ -2,7 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. #pragma once -#include + +#include +#include "irequesthandler.h" class ASPNET_CORE_PROXY_MODULE; @@ -10,8 +12,9 @@ class DisconnectHandler final: public IHttpConnectionStoredContext { public: DisconnectHandler() - : m_pModule(nullptr) + : m_pHandler(nullptr) { + InitializeSRWLock(&m_handlerLock); } virtual @@ -27,9 +30,10 @@ public: CleanupStoredContext() noexcept override; void - SetHandler(ASPNET_CORE_PROXY_MODULE * module) noexcept; + SetHandler(std::unique_ptr handler) noexcept; private: - std::atomic m_pModule; + SRWLOCK m_handlerLock {}; + std::unique_ptr m_pHandler; }; diff --git a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp index d3d71e8b4b..6f879f2e35 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp @@ -71,6 +71,10 @@ 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); @@ -112,8 +116,6 @@ 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)); @@ -121,6 +123,8 @@ 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 (...) { @@ -141,7 +145,7 @@ Finished: } } - return retVal; + return HandleNotificationStatus(retVal); } __override @@ -156,18 +160,27 @@ ASPNET_CORE_PROXY_MODULE::OnAsyncCompletion( { try { - return m_pHandler->OnAsyncCompletion( + return HandleNotificationStatus(m_pHandler->OnAsyncCompletion( pCompletionInfo->GetCompletionBytes(), - pCompletionInfo->GetCompletionStatus()); + pCompletionInfo->GetCompletionStatus())); } catch (...) { OBSERVE_CAUGHT_EXCEPTION(); - return RQ_NOTIFICATION_FINISH_REQUEST; + return HandleNotificationStatus(RQ_NOTIFICATION_FINISH_REQUEST); } } -void ASPNET_CORE_PROXY_MODULE::NotifyDisconnect() const +REQUEST_NOTIFICATION_STATUS ASPNET_CORE_PROXY_MODULE::HandleNotificationStatus(REQUEST_NOTIFICATION_STATUS status) noexcept { - m_pHandler->NotifyDisconnect(); + if (status != RQ_NOTIFICATION_PENDING) + { + if (m_pDisconnectHandler != nullptr) + { + m_pDisconnectHandler->SetHandler(nullptr); + m_pDisconnectHandler = nullptr; + } + } + + return status; } diff --git a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.h b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.h index 97c126b0f9..24f4794c68 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; - void - NotifyDisconnect() const; + REQUEST_NOTIFICATION_STATUS + HandleNotificationStatus(REQUEST_NOTIFICATION_STATUS status) noexcept; private: std::shared_ptr m_pApplicationManager; diff --git a/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.cpp b/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.cpp index 0bc46b8cf5..896189c8a8 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) +SRWExclusiveLock::SRWExclusiveLock(const SRWLOCK& lock) noexcept : m_lock(lock) { AcquireSRWLockExclusive(const_cast(&m_lock)); diff --git a/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h b/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h index 7ff977f70d..6c17e13111 100644 --- a/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h +++ b/src/AspNetCoreModuleV2/CommonLib/SRWExclusiveLock.h @@ -8,7 +8,7 @@ class SRWExclusiveLock { public: - SRWExclusiveLock(const SRWLOCK& lock); + SRWExclusiveLock(const SRWLOCK& lock) noexcept; ~SRWExclusiveLock(); private: const SRWLOCK& m_lock; diff --git a/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h b/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h index 8bd2076072..c89411f8a0 100644 --- a/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h +++ b/src/AspNetCoreModuleV2/CommonLib/irequesthandler.h @@ -54,3 +54,9 @@ 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 new file mode 100644 index 0000000000..9deeae3f92 --- /dev/null +++ b/test/Common.FunctionalTests/ClientDisconnectStress.cs @@ -0,0 +1,65 @@ +// 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 39d3b9f765..eb84272b76 100644 --- a/test/WebSites/InProcessWebSite/Startup.cs +++ b/test/WebSites/InProcessWebSite/Startup.cs @@ -324,6 +324,17 @@ 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) {