Merge QueryStatus and CreateHandler calls to make them atomic (#1359)

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();

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<APPLICATION_PARAMETER, 1> parameters {

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

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)),
@@ -24,7 +24,11 @@ public:
         InitializeSRWLock(&m_statusLock);
     }
     
-    APPLICATION_STATUS QueryStatus() override;
+    HRESULT
+    TryCreateHandler(
+        _In_ IHttpContext       *pHttpContext,
+        _Outptr_result_maybenull_ IREQUEST_HANDLER  **pRequestHandler) override;
+
     void CheckAppOffline();
     virtual HRESULT OnAppOfflineFound() = 0;
     void StopInternal(bool fServerInitiated) override { UNREFERENCED_PARAMETER(fServerInitiated); }

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)
     {

src/AspNetCoreModuleV2/AspNetCore/applicationinfo.cpp

@@ -14,52 +14,77 @@
 #include "AppOfflineApplication.h"
 
 HRESULT
-APPLICATION_INFO::GetOrCreateApplication(
+APPLICATION_INFO::CreateHandler(
     IHttpContext& pHttpContext,
-    std::unique_ptr<IAPPLICATION, IAPPLICATION_DELETER>& pApplication
+    std::unique_ptr<IREQUEST_HANDLER, IREQUEST_HANDLER_DELETER>& pHandler
 )
 {
     HRESULT             hr = S_OK;
 
-    SRWExclusiveLock lock(m_applicationLock);
-
-    auto& httpApplication = *pHttpContext.GetApplication();
-
-    if (m_pApplication != nullptr)
     {
-        if (m_pApplication->QueryStatus() == RECYCLED)
+        SRWSharedLock lock(m_applicationLock);
-        {
-            LOG_INFO(L"Application went offline");
         
-            // Call to wait for application to complete stopping
+        RETURN_IF_FAILED(hr = TryCreateHandler(pHttpContext, pHandler));
-            m_pApplication->Stop(/* fServerInitiated */ false);
+
-            m_pApplication = nullptr;
+        if (hr == S_OK)
-            m_pApplicationFactory = nullptr;
-        }
-        else
         {
-            // another thread created the application
+            return S_OK;
-            FINISHED(S_OK);
         }
     }
     
-    if (AppOfflineApplication::ShouldBeStarted(httpApplication))
+    {
+        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));
+        }
+    }
+
+    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<IREQUEST_HANDLER, IREQUEST_HANDLER_DELETER>& 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)

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,10 +47,9 @@ public:
     ShutDownApplication(bool fServerInitiated);
 
     HRESULT
-    GetOrCreateApplication(
+    CreateHandler(
         IHttpContext& pHttpContext,
-        std::unique_ptr<IAPPLICATION, IAPPLICATION_DELETER>& pApplication
+        std::unique_ptr<IREQUEST_HANDLER, IREQUEST_HANDLER_DELETER>& pHandler);
-    );
     
     bool ConfigurationPathApplies(const std::wstring& path)
     {
@@ -68,6 +67,15 @@ public:
     }
 
 private:
+    
+    HRESULT
+    TryCreateHandler(
+        IHttpContext& pHttpContext,
+        std::unique_ptr<IREQUEST_HANDLER, IREQUEST_HANDLER_DELETER>& pHandler);
+
+    HRESULT
+    CreateApplication(const IHttpApplication& pHttpApplication);
+
     IHttpServer            &m_pServer;
     HandlerResolver        &m_handlerResolver;
 

src/AspNetCoreModuleV2/AspNetCore/proxymodule.cpp

@@ -87,13 +87,7 @@ ASPNET_CORE_PROXY_MODULE::OnExecuteRequestHandler(
             *pHttpContext,
             m_pApplicationInfo));
 
-        std::unique_ptr<IAPPLICATION, IAPPLICATION_DELETER> pApplication;
+        FINISHED_IF_FAILED(m_pApplicationInfo->CreateHandler(*pHttpContext, m_pHandler));
-        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);
 
         retVal = m_pHandler->OnExecuteRequestHandler();
     }

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

src/AspNetCoreModuleV2/CommonLib/iapplication.h

@@ -6,12 +6,6 @@
 #include <memory>
 #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;
 };

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->
+    else if (m_pApplication->QueryBlockCallbacksIntoManaged())
-        QueryBlockCallbacksIntoManaged())
     {
         return ServerShutdownMessage();
     }

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
+    _In_ std::unique_ptr<OUT_OF_PROCESS_APPLICATION, IAPPLICATION_DELETER> pApplication
-) : IREQUEST_HANDLER(),
+) : 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<OUT_OF_PROCESS_APPLICATION*> (m_pApplication);
+    if (m_pApplication == NULL)
-    if (pApplication == NULL)
     {
         hr = E_INVALIDARG;
         goto Failure;
     }
 
-    hr = pApplication->GetProcess(&pServerProcess);
+    hr = m_pApplication->GetProcess(&pServerProcess);
     if (FAILED_LOG(hr))
     {
         fFailedToStartKestrel = TRUE;

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<OUT_OF_PROCESS_APPLICATION, IAPPLICATION_DELETER> pApplication
     );
 
     ~FORWARDING_HANDLER();
@@ -238,6 +238,6 @@ private:
 
     mutable LONG                        m_cRefs;
     IHttpContext*                       m_pW3Context;
-    OUT_OF_PROCESS_APPLICATION*         m_pApplication;
+    std::unique_ptr<OUT_OF_PROCESS_APPLICATION, IAPPLICATION_DELETER> m_pApplication;
     HTTP_MODULE_ID                      m_pModuleId;
 };

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;
 }