moving recycle and shutdown to applicationinfo (#639)

fix a race condition in processing request and app_offline, and optimize code logic

src/AspNetCore/Inc/applicationinfo.h

@@ -4,6 +4,8 @@
 #pragma once
 #define API_BUFFER_TOO_SMALL 0x80008098
 
+extern BOOL     g_fRecycleProcessCalled;
+
 typedef
 HRESULT
 (WINAPI * PFN_ASPNETCORE_CREATE_APPLICATION)(
@@ -128,19 +130,28 @@ public:
         return m_pConfiguration;
     }
 
-    APPLICATION*
+    //
-    QueryApplication()
+    // ExtractApplication will increase the reference counter of the application
+    // Caller is responsible for dereference the application.
+    // Otherwise memory leak
+    //
+    VOID
+    ExtractApplication(APPLICATION** ppApplication)
     {
-        return m_pApplication;
+        AcquireSRWLockShared(&m_srwLock);
+        if (m_pApplication != NULL)
+        {
+            m_pApplication->ReferenceApplication();
+        }
+        *ppApplication = m_pApplication;
+        ReleaseSRWLockShared(&m_srwLock);
     }
 
     VOID
-    ClearAndDereferenceApplication()
+    RecycleApplication();
-    {
+
-        APPLICATION* pApplication = m_pApplication;
+    VOID
-        m_pApplication = NULL;
+    ShutDownApplication();
-        pApplication->DereferenceApplication();
-    }
 
     HRESULT
     EnsureApplicationCreated();
@@ -156,6 +167,8 @@ private:
     HRESULT FindNativeAssemblyFromGlobalLocation(STRU* struFilename);
     HRESULT FindNativeAssemblyFromHostfxr(STRU* struFilename);
 
+    static VOID DoRecycleApplication(LPVOID lpParam);
+
     mutable LONG            m_cRefs;
     APPLICATION_INFO_KEY    m_applicationInfoKey;
     BOOL                    m_fAppOfflineFound;

src/AspNetCore/Inc/applicationmanager.h

@@ -55,18 +55,6 @@ public:
         _In_ PVOID                  pvContext
     );
 
-    static
-    VOID
-    RecycleApplication(
-        _In_ APPLICATION_INFO *     pEntry
-    );
-
-    static
-    void
-    DoRecycleApplication(
-        LPVOID lpParam
-    );
-
     static
     VOID
     ShutdownApplication(

src/AspNetCore/Src/applicationinfo.cpp

@@ -102,7 +102,8 @@ APPLICATION_INFO::UpdateAppOfflineFileHandle()
         &strFilePath);
     APP_OFFLINE_HTM *pOldAppOfflineHtm = NULL;
     APP_OFFLINE_HTM *pNewAppOfflineHtm = NULL;
-    BOOL             fLocked = FALSE;
+
+    ReferenceApplicationInfo();
 
     if (INVALID_FILE_ATTRIBUTES == GetFileAttributes(strFilePath.QueryStr()) &&
         GetLastError() == ERROR_FILE_NOT_FOUND)
@@ -156,13 +157,6 @@ APPLICATION_INFO::UpdateAppOfflineFileHandle()
         // recycle the application
         if (m_pApplication != NULL)
         {
-            // Lock here to avoid races with the application manager calling shutdown on the application.
-            AcquireSRWLockExclusive(&m_srwLock);
-            fLocked = TRUE;
-            if (m_pApplication == NULL)
-            {
-                goto Finished;
-            }
             STACK_STRU(strEventMsg, 256);
             if (SUCCEEDED(strEventMsg.SafeSnwprintf(
                 ASPNETCORE_EVENT_RECYCLE_APPOFFLINE_MSG,
@@ -174,16 +168,11 @@ APPLICATION_INFO::UpdateAppOfflineFileHandle()
                     strEventMsg.QueryStr());
             }
 
-            APPLICATION_MANAGER::RecycleApplication(this);
+            RecycleApplication();
-
         }
     }
 
-Finished:
+    DereferenceApplicationInfo();
-    if (fLocked)
-    {
-        ReleaseSRWLockExclusive(&m_srwLock);
-    }
 }
 
 HRESULT
@@ -239,19 +228,26 @@ APPLICATION_INFO::EnsureApplicationCreated()
             goto Finished;
         }
 
-        if (m_pfnAspNetCoreCreateApplication == NULL)
+        //
+        // in case of app offline, we don't want to create a new application now
+        //
+        if (!m_fAppOfflineFound)
         {
-            hr = HRESULT_FROM_WIN32(ERROR_INVALID_FUNCTION);
+            if (m_pfnAspNetCoreCreateApplication == NULL)
-            goto Finished;
+            {
-        }
+                hr = HRESULT_FROM_WIN32(ERROR_INVALID_FUNCTION);
+                goto Finished;
+            }
 
-        hr = m_pfnAspNetCoreCreateApplication(m_pServer, m_pConfiguration, &pApplication);
+            hr = m_pfnAspNetCoreCreateApplication(m_pServer, m_pConfiguration, &pApplication);
-        if (FAILED(hr))
+            if (FAILED(hr))
-        {
+            {
-            goto Finished;
+                goto Finished;
+            }
+            m_pApplication = pApplication;
         }
-        m_pApplication = pApplication;
     }
+
 Finished:
     if (fLocked)
     {
@@ -533,3 +529,110 @@ Finished:
     }
     return hr;
 }
+
+VOID
+APPLICATION_INFO::RecycleApplication()
+{
+    APPLICATION* pApplication = NULL;
+    HANDLE       hThread = INVALID_HANDLE_VALUE;
+    BOOL         fLockAcquired = FALSE;
+
+    if (m_pApplication != NULL)
+    {
+        AcquireSRWLockExclusive(&m_srwLock);
+        fLockAcquired = TRUE;
+        if (m_pApplication != NULL)
+        {
+            pApplication = m_pApplication;
+            if (pApplication->QueryConfig()->QueryHostingModel() == HOSTING_OUT_PROCESS)
+            {
+                //
+                // For inprocess, need to set m_pApplication to NULL first to
+                // avoid mapping new request to the recycled application.
+                // Outofprocess application instance will be created for new request
+                // For inprocess, as recycle will lead to shutdown later, leave m_pApplication
+                // to not block incoming requests till worker process shutdown
+                //
+                m_pApplication = NULL;
+            }
+            else
+            {
+                //
+                // For inprocess, need hold the application till shutdown is called
+                // Bump the reference counter as DoRecycleApplication will do dereference
+                //
+                pApplication->ReferenceApplication();
+            }
+
+            hThread = CreateThread(
+                NULL,       // default security attributes
+                0,          // default stack size
+                (LPTHREAD_START_ROUTINE)DoRecycleApplication,
+                pApplication,       // thread function arguments
+                0,          // default creation flags
+                NULL);      // receive thread identifier
+        }
+
+        if (hThread == NULL)
+        {
+            if (!g_fRecycleProcessCalled)
+            {
+                g_fRecycleProcessCalled = TRUE;
+                g_pHttpServer->RecycleProcess(L"On Demand by AspNetCore Module for recycle application failure");
+            }
+        }
+
+        if (fLockAcquired)
+        {
+            ReleaseSRWLockExclusive(&m_srwLock);
+        }
+    }
+}
+
+
+VOID
+APPLICATION_INFO::DoRecycleApplication(
+    LPVOID lpParam)
+{
+    APPLICATION* pApplication = static_cast<APPLICATION*>(lpParam);
+
+    // No lock required
+
+    if (pApplication != NULL)
+    {
+        // Recycle will call shutdown for out of process
+        pApplication->Recycle();
+
+        // Decrement the ref count as we reference it in RecycleApplication.
+        pApplication->DereferenceApplication();
+    }
+}
+
+
+VOID
+APPLICATION_INFO::ShutDownApplication()
+{
+    APPLICATION* pApplication = NULL;
+    BOOL         fLockAcquired = FALSE;
+
+    // pApplication can be NULL due to app_offline
+    if (m_pApplication != NULL)
+    {
+        AcquireSRWLockExclusive(&m_srwLock);
+        fLockAcquired = TRUE;
+        if (m_pApplication != NULL)
+        {
+            pApplication = m_pApplication;
+
+            // Set m_pApplication to NULL first to prevent anyone from using it
+            m_pApplication = NULL;
+            pApplication->ShutDown();
+            pApplication->DereferenceApplication();
+        }
+
+        if (fLockAcquired)
+        {
+            ReleaseSRWLockExclusive(&m_srwLock);
+        }
+    }
+}

src/AspNetCore/Src/applicationmanager.cxx

@@ -370,7 +370,7 @@ APPLICATION_MANAGER::RecycleApplicationFromManager(
             DBG_ASSERT(pRecord != NULL);
 
             // RecycleApplication is called on a separate thread. 
-            RecycleApplication(pRecord);
+            pRecord->RecycleApplication();
             pRecord->DereferenceApplicationInfo();
             path = context.MultiSz.Next(path);
         }
@@ -397,7 +397,11 @@ Finished:
                 strEventMsg.QueryStr());
         }
         // Need to recycle the process as we cannot recycle the application
-        g_pHttpServer->RecycleProcess(L"AspNetCore Recycle Process on Demand Due Application Recycle Error");
+        if (!g_fRecycleProcessCalled)
+        {
+            g_fRecycleProcessCalled = TRUE;
+            g_pHttpServer->RecycleProcess(L"AspNetCore Recycle Process on Demand Due Application Recycle Error");
+        }
     }
 
     return hr;
@@ -448,64 +452,7 @@ APPLICATION_MANAGER::ShutdownApplication(
 )
 {
     UNREFERENCED_PARAMETER(pvContext);
-    APPLICATION* pApplication = pEntry->QueryApplication();
+    DBG_ASSERT(pEntry != NULL);
-    pApplication->ReferenceApplication();
 
-    // Remove the application from the applicationInfo. 
+    pEntry->ShutDownApplication();
-    pEntry->ClearAndDereferenceApplication();
-    pApplication->ShutDown();
-    pApplication->DereferenceApplication();
-}
-
-//
-// Function used by DoRecycleApplication thread to do the real shutdown
-//
-// static
-VOID
-APPLICATION_MANAGER::DoRecycleApplication(
-    LPVOID lpParam)
-{
-    APPLICATION* pApplication = static_cast<APPLICATION*>(lpParam);
-
-    // Recycle will call shutdown for out of process
-    pApplication->Recycle();
-
-    // Decrement the ref count as we reference it in RecycleApplication.
-    pApplication->DereferenceApplication();
-}
-
-//
-// Function used to recycle an application
-//
-// static
-VOID
-APPLICATION_MANAGER::RecycleApplication(
-    _In_ APPLICATION_INFO *     pEntry
-)
-{
-    APPLICATION* pApplication = pEntry->QueryApplication();
-    DBG_ASSERT(pApplication != NULL);
-
-    // Reference the application first
-    pApplication->ReferenceApplication();
-
-    if (pApplication->QueryConfig()->QueryHostingModel() == HOSTING_OUT_PROCESS)
-    {
-        // Need to set m_pApplication to NULL first
-        // to avoid mapping new request to the recycled application
-        // A new application instance will be created for new request
-        pEntry->ClearAndDereferenceApplication();
-    }
-
-    // Reset application pointer to NULL
-    // The destructor of ApplictionInfo will not call ShutDown again
-    HANDLE hThread = CreateThread(
-        NULL,       // default security attributes
-        0,          // default stack size
-        (LPTHREAD_START_ROUTINE)DoRecycleApplication,
-        pApplication,       // thread function arguments
-        0,          // default creation flags
-        NULL);      // receive thread identifier
-
-    CloseHandle(hThread);
 }

src/AspNetCore/Src/proxymodule.cxx

@@ -138,18 +138,18 @@ ASPNET_CORE_PROXY_MODULE::OnExecuteRequestHandler(
     }
 
     // make sure assmebly is loaded and application is created
-
     hr = m_pApplicationInfo->EnsureApplicationCreated();
     if (FAILED(hr))
     {
         goto Finished;
     }
-    pApplication = m_pApplicationInfo->QueryApplication();
+
-    DBG_ASSERT(pApplication);
+    m_pApplicationInfo->ExtractApplication(&pApplication);
 
     // make sure application is in running state
     // cannot recreate the application as we cannot reload clr for inprocess
-    if (pApplication->QueryStatus() != APPLICATION_STATUS::RUNNING &&
+    if (pApplication != NULL &&
+        pApplication->QueryStatus() != APPLICATION_STATUS::RUNNING &&
         pApplication->QueryStatus() != APPLICATION_STATUS::STARTING)
     {
         hr = HRESULT_FROM_WIN32(ERROR_SERVER_DISABLED);
@@ -166,6 +166,7 @@ ASPNET_CORE_PROXY_MODULE::OnExecuteRequestHandler(
     {
         goto Finished;
     }
+
     retVal = m_pHandler->OnExecuteRequestHandler();
 
 Finished: 
@@ -175,6 +176,10 @@ Finished:
         retVal = RQ_NOTIFICATION_FINISH_REQUEST;
     }
 
+    if (pApplication != NULL)
+    {
+        pApplication->DereferenceApplication();
+    }
     return retVal;
 }