From 2597a2e0098daedf13787beb26f8de4d370848a1 Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Thu, 6 Sep 2018 15:42:18 -0700 Subject: [PATCH] Merge QueryStatus and CreateHandler calls to make them atomic (#1359) --- .../AspNetCore/AppOfflineApplication.h | 2 +- .../AspNetCore/ApplicationFactory.h | 4 +- .../PollingAppOfflineApplication.cpp | 9 +- .../AspNetCore/PollingAppOfflineApplication.h | 8 +- .../AspNetCore/ServerErrorApplication.h | 2 +- .../AspNetCore/applicationinfo.cpp | 100 ++++++++++++------ .../AspNetCore/applicationinfo.h | 20 ++-- .../AspNetCore/proxymodule.cpp | 8 +- .../CommonLib/application.h | 27 +++-- .../CommonLib/iapplication.h | 12 +-- .../inprocesshandler.cpp | 3 +- .../forwardinghandler.cpp | 12 +-- .../forwardinghandler.h | 4 +- .../outprocessapplication.cpp | 2 +- 14 files changed, 132 insertions(+), 81 deletions(-) diff --git a/src/AspNetCoreModuleV2/AspNetCore/AppOfflineApplication.h b/src/AspNetCoreModuleV2/AspNetCore/AppOfflineApplication.h index 14399edaaa..2c81ae98b1 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/AppOfflineApplication.h +++ b/src/AspNetCoreModuleV2/AspNetCore/AppOfflineApplication.h @@ -10,7 +10,7 @@ class AppOfflineApplication: public PollingAppOfflineApplication { public: - AppOfflineApplication(IHttpApplication& pApplication) + AppOfflineApplication(const IHttpApplication& pApplication) : PollingAppOfflineApplication(pApplication, PollingAppOfflineApplicationMode::StopWhenRemoved) { CheckAppOffline(); diff --git a/src/AspNetCoreModuleV2/AspNetCore/ApplicationFactory.h b/src/AspNetCoreModuleV2/AspNetCore/ApplicationFactory.h index 9d310438ef..30a0a3359b 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/ApplicationFactory.h +++ b/src/AspNetCoreModuleV2/AspNetCore/ApplicationFactory.h @@ -13,7 +13,7 @@ typedef HRESULT (WINAPI * PFN_ASPNETCORE_CREATE_APPLICATION)( _In_ IHttpServer *pServer, - _In_ IHttpApplication *pHttpApplication, + _In_ const IHttpApplication * pHttpApplication, _In_ APPLICATION_PARAMETER *pParameters, _In_ DWORD nParameters, _Out_ IAPPLICATION **pApplication @@ -31,7 +31,7 @@ public: HRESULT Execute( _In_ IHttpServer *pServer, - _In_ IHttpApplication *pHttpApplication, + _In_ const IHttpApplication *pHttpApplication, _Outptr_ IAPPLICATION **pApplication) const noexcept { std::array parameters { diff --git a/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.cpp b/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.cpp index 382950fde4..cf2395e7b5 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.cpp @@ -8,15 +8,20 @@ #include "HandleWrapper.h" #include "exceptions.h" -APPLICATION_STATUS PollingAppOfflineApplication::QueryStatus() +HRESULT PollingAppOfflineApplication::TryCreateHandler(_In_ IHttpContext* pHttpContext, _Outptr_result_maybenull_ IREQUEST_HANDLER** pRequestHandler) { CheckAppOffline(); - return APPLICATION::QueryStatus(); + return LOG_IF_FAILED(APPLICATION::TryCreateHandler(pHttpContext, pRequestHandler)); } void PollingAppOfflineApplication::CheckAppOffline() { + if (m_fStopCalled) + { + return; + } + const auto ulCurrentTime = GetTickCount64(); // // we only care about app offline presented. If not, it means the application has started diff --git a/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.h b/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.h index eadd0da3b8..0f0eb58c8c 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.h +++ b/src/AspNetCoreModuleV2/AspNetCore/PollingAppOfflineApplication.h @@ -14,7 +14,7 @@ enum PollingAppOfflineApplicationMode class PollingAppOfflineApplication: public APPLICATION { public: - PollingAppOfflineApplication(IHttpApplication& pApplication, PollingAppOfflineApplicationMode mode) + PollingAppOfflineApplication(const IHttpApplication& pApplication, PollingAppOfflineApplicationMode mode) : APPLICATION(pApplication), m_ulLastCheckTime(0), m_appOfflineLocation(GetAppOfflineLocation(pApplication)), @@ -23,8 +23,12 @@ public: { InitializeSRWLock(&m_statusLock); } + + HRESULT + TryCreateHandler( + _In_ IHttpContext *pHttpContext, + _Outptr_result_maybenull_ IREQUEST_HANDLER **pRequestHandler) override; - APPLICATION_STATUS QueryStatus() override; void CheckAppOffline(); virtual HRESULT OnAppOfflineFound() = 0; void StopInternal(bool fServerInitiated) override { UNREFERENCED_PARAMETER(fServerInitiated); } diff --git a/src/AspNetCoreModuleV2/AspNetCore/ServerErrorApplication.h b/src/AspNetCoreModuleV2/AspNetCore/ServerErrorApplication.h index 86a9a1a0e3..df686adbc4 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/ServerErrorApplication.h +++ b/src/AspNetCoreModuleV2/AspNetCore/ServerErrorApplication.h @@ -9,7 +9,7 @@ class ServerErrorApplication : public PollingAppOfflineApplication { public: - ServerErrorApplication(IHttpApplication& pApplication, HRESULT hr) + ServerErrorApplication(const IHttpApplication& pApplication, HRESULT hr) : m_HR(hr), PollingAppOfflineApplication(pApplication, PollingAppOfflineApplicationMode::StopWhenAdded) { diff --git a/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.cpp b/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.cpp index 225a82bdec..85bb5c6398 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.cpp @@ -14,52 +14,77 @@ #include "AppOfflineApplication.h" HRESULT -APPLICATION_INFO::GetOrCreateApplication( +APPLICATION_INFO::CreateHandler( IHttpContext& pHttpContext, - std::unique_ptr& pApplication + std::unique_ptr& pHandler ) { HRESULT hr = S_OK; - SRWExclusiveLock lock(m_applicationLock); - - auto& httpApplication = *pHttpContext.GetApplication(); - - if (m_pApplication != nullptr) { - if (m_pApplication->QueryStatus() == RECYCLED) - { - LOG_INFO(L"Application went offline"); + SRWSharedLock lock(m_applicationLock); + + RETURN_IF_FAILED(hr = TryCreateHandler(pHttpContext, pHandler)); - // Call to wait for application to complete stopping - m_pApplication->Stop(/* fServerInitiated */ false); - m_pApplication = nullptr; - m_pApplicationFactory = nullptr; - } - else + if (hr == S_OK) { - // another thread created the application - FINISHED(S_OK); + return S_OK; + } + } + + { + SRWExclusiveLock lock(m_applicationLock); + + // check if other thread created application + RETURN_IF_FAILED(hr = TryCreateHandler(pHttpContext, pHandler)); + + // In some cases (adding and removing app_offline quickly) application might start and stop immediately + // so retry until we get valid handler or error + while (hr != S_OK) + { + // At this point application is either null or shutdown and is returning S_FALSE + + if (m_pApplication != nullptr) + { + LOG_INFO(L"Application went offline"); + + // Call to wait for application to complete stopping + m_pApplication->Stop(/* fServerInitiated */ false); + m_pApplication = nullptr; + m_pApplicationFactory = nullptr; + } + + RETURN_IF_FAILED(CreateApplication(*pHttpContext.GetApplication())); + + RETURN_IF_FAILED(hr = TryCreateHandler(pHttpContext, pHandler)); } } - if (AppOfflineApplication::ShouldBeStarted(httpApplication)) + return S_OK; +} + +HRESULT +APPLICATION_INFO::CreateApplication(const IHttpApplication& pHttpApplication) +{ + HRESULT hr = S_OK; + + if (AppOfflineApplication::ShouldBeStarted(pHttpApplication)) { LOG_INFO(L"Detected app_offline file, creating polling application"); #pragma warning( push ) #pragma warning ( disable : 26409 ) // Disable "Avoid using new", using custom deleter here - m_pApplication.reset(new AppOfflineApplication(httpApplication)); + m_pApplication.reset(new AppOfflineApplication(pHttpApplication)); #pragma warning( pop ) } else { - FINISHED_IF_FAILED(m_handlerResolver.GetApplicationFactory(httpApplication, m_pApplicationFactory)); + FINISHED_IF_FAILED(m_handlerResolver.GetApplicationFactory(pHttpApplication, m_pApplicationFactory)); LOG_INFO(L"Creating handler application"); IAPPLICATION * newApplication; FINISHED_IF_FAILED(m_pApplicationFactory->Execute( &m_pServer, - &httpApplication, + &pHttpApplication, &newApplication)); m_pApplication.reset(newApplication); @@ -67,29 +92,44 @@ APPLICATION_INFO::GetOrCreateApplication( Finished: - if (FAILED(hr)) + if (m_pApplication == nullptr || FAILED(hr)) { // Log the failure and update application info to not try again EventLog::Error( ASPNETCORE_EVENT_ADD_APPLICATION_ERROR, ASPNETCORE_EVENT_ADD_APPLICATION_ERROR_MSG, - httpApplication.GetApplicationId(), + pHttpApplication.GetApplicationId(), hr); #pragma warning( push ) #pragma warning ( disable : 26409 ) // Disable "Avoid using new", using custom deleter here - m_pApplication.reset(new ServerErrorApplication(httpApplication, hr)); + m_pApplication.reset(new ServerErrorApplication(pHttpApplication, hr)); #pragma warning( pop ) } - if (m_pApplication) - { - pApplication = ReferenceApplication(m_pApplication.get()); - } - return hr; } +HRESULT +APPLICATION_INFO::TryCreateHandler( + IHttpContext& pHttpContext, + std::unique_ptr& pHandler) +{ + if (m_pApplication != nullptr) + { + IREQUEST_HANDLER * newHandler; + const auto result = m_pApplication->TryCreateHandler(&pHttpContext, &newHandler); + RETURN_IF_FAILED(result); + + if (result == S_OK) + { + pHandler.reset(newHandler); + // another thread created the application + return S_OK; + } + } + return S_FALSE; +} VOID APPLICATION_INFO::ShutDownApplication(bool fServerInitiated) diff --git a/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.h b/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.h index 998cdc5ae1..9c6d1b7cde 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.h +++ b/src/AspNetCoreModuleV2/AspNetCore/applicationinfo.h @@ -32,13 +32,13 @@ public: ~APPLICATION_INFO() = default; const std::wstring& - QueryApplicationInfoKey() const noexcept + QueryApplicationInfoKey() noexcept { return m_strInfoKey; } const std::wstring& - QueryConfigPath() const noexcept + QueryConfigPath() noexcept { return m_strConfigPath; } @@ -47,11 +47,10 @@ public: ShutDownApplication(bool fServerInitiated); HRESULT - GetOrCreateApplication( + CreateHandler( IHttpContext& pHttpContext, - std::unique_ptr& pApplication - ); - + std::unique_ptr& pHandler); + bool ConfigurationPathApplies(const std::wstring& path) { // We need to check that the last character of the config path @@ -68,6 +67,15 @@ public: } private: + + HRESULT + TryCreateHandler( + IHttpContext& pHttpContext, + std::unique_ptr& pHandler); + + HRESULT + CreateApplication(const IHttpApplication& pHttpApplication); + IHttpServer &m_pServer; HandlerResolver &m_handlerResolver; diff --git a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp index c93c7c7602..8fafe13c1a 100644 --- a/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp +++ b/src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp @@ -87,13 +87,7 @@ ASPNET_CORE_PROXY_MODULE::OnExecuteRequestHandler( *pHttpContext, m_pApplicationInfo)); - std::unique_ptr pApplication; - FINISHED_IF_FAILED(m_pApplicationInfo->GetOrCreateApplication(*pHttpContext, pApplication)); - - IREQUEST_HANDLER* pHandler; - // Create RequestHandler and process the request - FINISHED_IF_FAILED(pApplication->CreateHandler(pHttpContext, &pHandler)); - m_pHandler.reset(pHandler); + FINISHED_IF_FAILED(m_pApplicationInfo->CreateHandler(*pHttpContext, m_pHandler)); retVal = m_pHandler->OnExecuteRequestHandler(); } diff --git a/src/AspNetCoreModuleV2/CommonLib/application.h b/src/AspNetCoreModuleV2/CommonLib/application.h index c3d8eeb898..814be8aae6 100644 --- a/src/AspNetCoreModuleV2/CommonLib/application.h +++ b/src/AspNetCoreModuleV2/CommonLib/application.h @@ -7,7 +7,6 @@ #include "iapplication.h" #include "ntassert.h" #include "SRWExclusiveLock.h" -#include "SRWSharedLock.h" class APPLICATION : public IAPPLICATION { @@ -16,6 +15,26 @@ public: APPLICATION(const APPLICATION&) = delete; const APPLICATION& operator=(const APPLICATION&) = delete; + HRESULT + TryCreateHandler( + _In_ IHttpContext *pHttpContext, + _Outptr_result_maybenull_ IREQUEST_HANDLER **pRequestHandler) override + { + *pRequestHandler = nullptr; + + if (m_fStopCalled) + { + return S_FALSE; + } + + return CreateHandler(pHttpContext, pRequestHandler); + } + + virtual + HRESULT + CreateHandler( + _In_ IHttpContext *pHttpContext, + _Outptr_opt_ IREQUEST_HANDLER **pRequestHandler) = 0; APPLICATION(const IHttpApplication& pHttpApplication) : m_fStopCalled(false), @@ -28,12 +47,6 @@ public: m_applicationVirtualPath = ToVirtualPath(m_applicationConfigPath); } - APPLICATION_STATUS - QueryStatus() override - { - SRWSharedLock stateLock(m_stateLock); - return m_fStopCalled ? APPLICATION_STATUS::RECYCLED : APPLICATION_STATUS::RUNNING; - } VOID Stop(bool fServerInitiated) override diff --git a/src/AspNetCoreModuleV2/CommonLib/iapplication.h b/src/AspNetCoreModuleV2/CommonLib/iapplication.h index 7c205ed68c..3349cb66fb 100644 --- a/src/AspNetCoreModuleV2/CommonLib/iapplication.h +++ b/src/AspNetCoreModuleV2/CommonLib/iapplication.h @@ -6,12 +6,6 @@ #include #include "irequesthandler.h" -enum APPLICATION_STATUS -{ - RUNNING, - RECYCLED, -}; - struct APPLICATION_PARAMETER { LPCSTR pzName; @@ -28,10 +22,6 @@ public: virtual ~IAPPLICATION() = 0 { }; - virtual - APPLICATION_STATUS - QueryStatus() = 0; - virtual VOID ReferenceApplication() = 0; @@ -42,7 +32,7 @@ public: virtual HRESULT - CreateHandler( + TryCreateHandler( _In_ IHttpContext *pHttpContext, _Outptr_ IREQUEST_HANDLER **pRequestHandler) = 0; }; diff --git a/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp b/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp index 0b7357f5d2..9e7e84d7f1 100644 --- a/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp +++ b/src/AspNetCoreModuleV2/InProcessRequestHandler/inprocesshandler.cpp @@ -59,8 +59,7 @@ IN_PROCESS_HANDLER::OnExecuteRequestHandler() return RQ_NOTIFICATION_FINISH_REQUEST; } - else if (m_pApplication->QueryStatus() != APPLICATION_STATUS::RUNNING || m_pApplication-> - QueryBlockCallbacksIntoManaged()) + else if (m_pApplication->QueryBlockCallbacksIntoManaged()) { return ServerShutdownMessage(); } diff --git a/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.cpp b/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.cpp index b0fa1aa03f..6dd018783b 100644 --- a/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.cpp +++ b/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.cpp @@ -24,8 +24,8 @@ RESPONSE_HEADER_HASH * FORWARDING_HANDLER::sm_pResponseHeaderHash = NULL; FORWARDING_HANDLER::FORWARDING_HANDLER( _In_ IHttpContext *pW3Context, - _In_ OUT_OF_PROCESS_APPLICATION *pApplication -) : IREQUEST_HANDLER(), + _In_ std::unique_ptr pApplication +) : REQUEST_HANDLER(), m_Signature(FORWARDING_HANDLER_SIGNATURE), m_RequestStatus(FORWARDER_START), m_fClientDisconnected(FALSE), @@ -46,7 +46,7 @@ FORWARDING_HANDLER::FORWARDING_HANDLER( m_fServerResetConn(FALSE), m_cRefs(1), m_pW3Context(pW3Context), - m_pApplication(pApplication) + m_pApplication(std::move(pApplication)) { LOG_TRACE(L"FORWARDING_HANDLER::FORWARDING_HANDLER"); @@ -100,7 +100,6 @@ FORWARDING_HANDLER::OnExecuteRequestHandler() IHttpRequest *pRequest = m_pW3Context->GetRequest(); IHttpResponse *pResponse = m_pW3Context->GetResponse(); IHttpConnection *pClientConnection = NULL; - OUT_OF_PROCESS_APPLICATION *pApplication = NULL; PROTOCOL_CONFIG *pProtocol = &sm_ProtocolConfig; SERVER_PROCESS *pServerProcess = NULL; @@ -128,14 +127,13 @@ FORWARDING_HANDLER::OnExecuteRequestHandler() goto Failure; } - pApplication = static_cast (m_pApplication); - if (pApplication == NULL) + if (m_pApplication == NULL) { hr = E_INVALIDARG; goto Failure; } - hr = pApplication->GetProcess(&pServerProcess); + hr = m_pApplication->GetProcess(&pServerProcess); if (FAILED_LOG(hr)) { fFailedToStartKestrel = TRUE; diff --git a/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.h b/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.h index 02516c4f71..a41b86fe50 100644 --- a/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.h +++ b/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.h @@ -22,7 +22,7 @@ class FORWARDING_HANDLER : public REQUEST_HANDLER public: FORWARDING_HANDLER( _In_ IHttpContext *pW3Context, - _In_ OUT_OF_PROCESS_APPLICATION *pApplication + _In_ std::unique_ptr pApplication ); ~FORWARDING_HANDLER(); @@ -238,6 +238,6 @@ private: mutable LONG m_cRefs; IHttpContext* m_pW3Context; - OUT_OF_PROCESS_APPLICATION* m_pApplication; + std::unique_ptr m_pApplication; HTTP_MODULE_ID m_pModuleId; }; diff --git a/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/outprocessapplication.cpp b/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/outprocessapplication.cpp index 8d3738d82f..22d0da50c9 100644 --- a/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/outprocessapplication.cpp +++ b/src/AspNetCoreModuleV2/OutOfProcessRequestHandler/outprocessapplication.cpp @@ -72,7 +72,7 @@ OUT_OF_PROCESS_APPLICATION::CreateHandler( SetWebsocketStatus(pHttpContext); } - pHandler = new FORWARDING_HANDLER(pHttpContext, this); + pHandler = new FORWARDING_HANDLER(pHttpContext, ::ReferenceApplication(this)); *pRequestHandler = pHandler; return S_OK; }