Collection of fixes for various shutdown scenario race conditions

lib/api/LogManagerImpl.cpp

@@ -292,10 +292,9 @@ namespace ARIASDK_NS_BEGIN
     void LogManagerImpl::FlushAndTeardown()
     {
         LOG_INFO("Shutting down...");
-
+        LOCKGUARD(m_lock);
+        if (m_alive)
         {
-            LOCKGUARD(m_lock);
-
             LOG_INFO("Tearing down modules");
             TeardownModules();
 
@@ -315,15 +314,14 @@ namespace ARIASDK_NS_BEGIN
             m_httpClient = nullptr;
             m_taskDispatcher = nullptr;
             m_dataViewer = nullptr;
-        }
 
-        m_filters.UnregisterAllFilters();
-
-        auto shutTime = GetUptimeMs();
-        PAL::shutdown();
-        shutTime = GetUptimeMs() - shutTime;
-        LOG_INFO("Shutdown complete in %lld ms", shutTime);
+            m_filters.UnregisterAllFilters();
 
+            auto shutTime = GetUptimeMs();
+            PAL::shutdown();
+            shutTime = GetUptimeMs() - shutTime;
+            LOG_INFO("Shutdown complete in %lld ms", shutTime);
+        }
         m_alive = false;
 
     }

lib/http/HttpClientManager.cpp

@@ -106,6 +106,7 @@ namespace ARIASDK_NS_BEGIN {
         PAL::scheduleTask(&m_taskDispatcher, 0, this, &HttpClientManager::onHttpResponse, callback);
     }
 
+    /* This method may get executed synchronously on Windows from handleSendRequest in case of connection failure */
     void HttpClientManager::onHttpResponse(HttpCallback* callback)
     {
         EventsUploadContextPtr &ctx = callback->m_ctx;

lib/http/HttpClientManager.hpp

@@ -51,7 +51,7 @@ class HttpClientManager
         ILogManager&              m_logManager;
         IHttpClient&              m_httpClient;
         ITaskDispatcher&          m_taskDispatcher;
-        std::mutex                m_httpCallbacksMtx;
+        std::recursive_mutex      m_httpCallbacksMtx;
         std::list<HttpCallback*>  m_httpCallbacks;
 };
 

lib/http/HttpClient_WinInet.cpp

@@ -1,3 +1,4 @@
+// clang-format off
 // Copyright (c) Microsoft. All rights reserved.
 #include "mat/config.h"
 
@@ -11,6 +12,7 @@
 
 #include <algorithm>
 #include <memory>
+#include <mutex>
 #include <sstream>
 #include <vector>
 #include <oacr.h>
@@ -22,17 +24,21 @@ class WinInetRequestWrapper
   protected:
     HttpClient_WinInet&    m_parent;
     std::string            m_id;
-    IHttpResponseCallback* m_appCallback {};
-    HINTERNET              m_hWinInetSession {};
-    HINTERNET              m_hWinInetRequest {};
+    IHttpResponseCallback* m_appCallback{ nullptr };
+    HINTERNET              m_hWinInetSession { nullptr };
+    HINTERNET              m_hWinInetRequest { nullptr };
     SimpleHttpRequest*     m_request;
     BYTE                   m_buffer[1024] {};
+    std::recursive_mutex   m_connectingMutex;
+    bool                   isAborted { false };
+    bool                   isCallbackCalled { false };
 
   public:
     WinInetRequestWrapper(HttpClient_WinInet& parent, SimpleHttpRequest* request)
       : m_parent(parent),
-        m_request(request),
-        m_id(request->GetId())
+        m_id(request->GetId()),
+        m_hWinInetRequest(nullptr),
+        m_request(request)
     {
         LOG_TRACE("%p WinInetRequestWrapper()", this);
     }
@@ -55,10 +61,13 @@ class WinInetRequestWrapper
      */
     void cancel()
     {
+        LOCKGUARD(m_connectingMutex);
+        // Cover the case where m_hWinInetRequest handle hasn't been created yet:
+        isAborted = true;
+        // Cover the case where m_hWinInetRequest handle is created and has a callback registered:
         if (m_hWinInetRequest != nullptr)
         {
             ::InternetCloseHandle(m_hWinInetRequest);
-            // don't wait for request callback
         }
     }
 
@@ -112,71 +121,123 @@ class WinInetRequestWrapper
 
     void send(IHttpResponseCallback* callback)
     {
+        // Register app callback and request in HttpClient map
         m_appCallback = callback;
 
         {
             std::lock_guard<std::mutex> lock(m_parent.m_requestsMutex);
             m_parent.m_requests[m_id] = this;
         }
 
-        URL_COMPONENTSA urlc;
-        memset(&urlc, 0, sizeof(urlc));
-        urlc.dwStructSize = sizeof(urlc);
-        char hostname[256];
-        urlc.lpszHostName = hostname;
-        urlc.dwHostNameLength = sizeof(hostname);
-        char path[1024];
-        urlc.lpszUrlPath = path;
-        urlc.dwUrlPathLength = sizeof(path);
-        if (!::InternetCrackUrlA(m_request->m_url.data(), (DWORD)m_request->m_url.size(), 0, &urlc)) {
-            DWORD dwError = ::GetLastError();
-            LOG_WARN("InternetCrackUrl() failed: dwError=%d url=%s", dwError, m_request->m_url.data());
-            onRequestComplete(dwError);
-            return;
-        }
+        {   // Section of code that uses m_connectingMutex lock without a LOCKGUARD macro:
+            // - don't forget to unlock the mutex before calling "onRequestComplete" which destroys this object
+            // - don't access any object members after "onRequestComplete" (object is dead at this point)
+            m_connectingMutex.lock();
 
-        m_hWinInetSession = ::InternetConnectA(m_parent.m_hInternet, hostname, urlc.nPort,
-            NULL, NULL, INTERNET_SERVICE_HTTP, 0, reinterpret_cast<DWORD_PTR>(this));
-        if (m_hWinInetSession == NULL) {
-            DWORD dwError = ::GetLastError();
-            LOG_WARN("InternetConnect() failed: %d", dwError);
-            onRequestComplete(dwError);
-            return;
-        }
-        // TODO: Session handle for the same target should be cached across requests to enable keep-alive.
-
-        PCSTR szAcceptTypes[] = {"*/*", NULL};
-        m_hWinInetRequest = ::HttpOpenRequestA(
-            m_hWinInetSession, m_request->m_method.c_str(), path, NULL, NULL, szAcceptTypes,
-            INTERNET_FLAG_KEEP_CONNECTION | INTERNET_FLAG_NO_AUTH | INTERNET_FLAG_NO_CACHE_WRITE |
-            INTERNET_FLAG_NO_COOKIES | INTERNET_FLAG_NO_UI | INTERNET_FLAG_PRAGMA_NOCACHE |
-            INTERNET_FLAG_RELOAD | (urlc.nScheme == INTERNET_SCHEME_HTTPS ? INTERNET_FLAG_SECURE : 0),
-            reinterpret_cast<DWORD_PTR>(this));
-        if (m_hWinInetRequest == NULL) {
-            DWORD dwError = ::GetLastError();
-            LOG_WARN("HttpOpenRequest() failed: %d", dwError);
-            onRequestComplete(dwError);
-            return;
-        }
+            // If outside code asked us to abort that request before we could proceed with creating a WinInet
+            // handle on it, then clean it right away before proceeding with any async WinInet API calls.
+            if (isAborted)
+            {
+                // Request force-aborted before creating a WinInet handle.
+                DispatchEvent(OnConnectFailed);
+                m_connectingMutex.unlock();
+                onRequestComplete(ERROR_INTERNET_OPERATION_CANCELLED);
+                return;
+            }
 
-        /* Perform optional MS Root certificate check for certain end-point URLs */
-        if (m_parent.IsMsRootCheckRequired())
-        {
-            if (!isMsRootCert())
+            DispatchEvent(OnConnecting);
+
+            URL_COMPONENTSA urlc;
+            memset(&urlc, 0, sizeof(urlc));
+            urlc.dwStructSize = sizeof(urlc);
+            char hostname[256];
+            urlc.lpszHostName = hostname;
+            urlc.dwHostNameLength = sizeof(hostname);
+            char path[1024];
+            urlc.lpszUrlPath = path;
+            urlc.dwUrlPathLength = sizeof(path);
+            if (!::InternetCrackUrlA(m_request->m_url.data(), (DWORD)m_request->m_url.size(), 0, &urlc))
             {
-                onRequestComplete(ERROR_INTERNET_SEC_INVALID_CERT);
+                DWORD dwError = ::GetLastError();
+                LOG_WARN("InternetCrackUrl() failed: dwError=%d url=%s", dwError, m_request->m_url.data());
+                // Invalid URL passed to WinInet API
+                DispatchEvent(OnConnectFailed);
+                m_connectingMutex.unlock();
+                onRequestComplete(ERROR_INTERNET_OPERATION_CANCELLED);
+                return;
+            }
+
+            m_hWinInetSession = ::InternetConnectA(m_parent.m_hInternet, hostname, urlc.nPort,
+                NULL, NULL, INTERNET_SERVICE_HTTP, 0, reinterpret_cast<DWORD_PTR>(this));
+            if (m_hWinInetSession == NULL)
+            {
+                DWORD dwError = ::GetLastError();
+                LOG_WARN("InternetConnect() failed: %d", dwError);
+                // Cannot connect to host
+                DispatchEvent(OnConnectFailed);
+                m_connectingMutex.unlock();
+                onRequestComplete(ERROR_INTERNET_OPERATION_CANCELLED);
+                return;
+            }
+            // TODO: Session handle for the same target should be cached across requests to enable keep-alive.
+
+            PCSTR szAcceptTypes[] = {"*/*", NULL};
+            m_hWinInetRequest = ::HttpOpenRequestA(
+                m_hWinInetSession, m_request->m_method.c_str(), path, NULL, NULL, szAcceptTypes,
+                INTERNET_FLAG_KEEP_CONNECTION | INTERNET_FLAG_NO_AUTH | INTERNET_FLAG_NO_CACHE_WRITE |
+                INTERNET_FLAG_NO_COOKIES | INTERNET_FLAG_NO_UI | INTERNET_FLAG_PRAGMA_NOCACHE |
+                INTERNET_FLAG_RELOAD | (urlc.nScheme == INTERNET_SCHEME_HTTPS ? INTERNET_FLAG_SECURE : 0),
+                reinterpret_cast<DWORD_PTR>(this));
+            if (m_hWinInetRequest == NULL)
+            {
+                DWORD dwError = ::GetLastError();
+                LOG_WARN("HttpOpenRequest() failed: %d", dwError);
+                // Request cannot be opened to given URL because of some connectivity issue
+                DispatchEvent(OnConnectFailed);
+                m_connectingMutex.unlock();
+                onRequestComplete(ERROR_INTERNET_OPERATION_CANCELLED);
                 return;
             }
-        }
 
-        ::InternetSetStatusCallback(m_hWinInetRequest, &WinInetRequestWrapper::winInetCallback);
+            /* Perform optional MS Root certificate check for certain end-point URLs */
+            if (m_parent.IsMsRootCheckRequired())
+            {
+                if (!isMsRootCert())
+                {
+                    // Request cannot be completed: end-point certificate is not MS-Rooted.
+                    DispatchEvent(OnConnectFailed);
+                    m_connectingMutex.unlock();
+                    onRequestComplete(ERROR_INTERNET_SEC_INVALID_CERT);
+                    return;
+                }
+            }
+
+            ::InternetSetStatusCallback(m_hWinInetRequest, &WinInetRequestWrapper::winInetCallback);
+            // At this point this->cancel() is able to async-destroy the request object by calling
+            // ::InternetCloseHandle(...) : winInetCallback is invoked in context of WinInet thread
+            // pool worker thread, which subsequently calls onRequestComplete(...) to destroy.
+            m_connectingMutex.unlock();
+        }
+        // End of section of code that uses m_connectingMutex lock
 
         std::ostringstream os;
-        for (auto const& header : m_request->m_headers) {
+        for (auto const& header : m_request->m_headers)
+        {
             os << header.first << ": " << header.second << "\r\n";
         }
-        ::HttpAddRequestHeadersA(m_hWinInetRequest, os.str().data(), static_cast<DWORD>(os.tellp()), HTTP_ADDREQ_FLAG_ADD | HTTP_ADDREQ_FLAG_REPLACE);
+        if (!::HttpAddRequestHeadersA(m_hWinInetRequest, os.str().data(), static_cast<DWORD>(os.tellp()), HTTP_ADDREQ_FLAG_ADD | HTTP_ADDREQ_FLAG_REPLACE))
+        {
+            DWORD dwError = ::GetLastError();
+            LOG_WARN("HttpAddRequestHeadersA() failed: %d", dwError);
+            // Unable to add request headers. There's no point in proceeding with upload because
+            // our server is expecting those custom request headers to always be there.
+            DispatchEvent(OnConnectFailed);
+            onRequestComplete(ERROR_INTERNET_OPERATION_CANCELLED);
+            return;
+        }
 
+        // Try to send headers and request body to server
+        DispatchEvent(OnSending);
         void *data = static_cast<void *>(m_request->m_body.data());
         DWORD size = static_cast<DWORD>(m_request->m_body.size());
         BOOL bResult = ::HttpSendRequest(m_hWinInetRequest, NULL, 0, data, (DWORD)size);
@@ -185,8 +246,12 @@ class WinInetRequestWrapper
         if (bResult == TRUE && dwError != ERROR_IO_PENDING) {
             dwError = ::GetLastError();
             LOG_WARN("HttpSendRequest() failed: %d", dwError);
-            onRequestComplete(dwError);
+            // Unable to send requerst
+            DispatchEvent(OnSendFailed);
+            onRequestComplete(ERROR_INTERNET_OPERATION_CANCELLED);
+            return;
         }
+        // Async request has been  queued in WinInet thread pool
     }
 
     static void CALLBACK winInetCallback(HINTERNET hInternet, DWORD_PTR dwContext, DWORD dwInternetStatus, LPVOID lpvStatusInformation, DWORD dwStatusInformationLength)
@@ -207,6 +272,12 @@ class WinInetRequestWrapper
                 return;
             }
 
+            case INTERNET_STATUS_HANDLE_CLOSING:
+                // HANDLE_CLOSING should always come after REQUEST_COMPLETE/. When (and if)
+                // it (ever) happens, WinInetRequestWrapper* self pointer may point to object
+                // that has been already destroyed. We do not perform any actions on it.
+                return;
+
             case INTERNET_STATUS_REQUEST_COMPLETE: {
                 assert(dwStatusInformationLength >= sizeof(INTERNET_ASYNC_RESULT));
                 INTERNET_ASYNC_RESULT& result = *static_cast<INTERNET_ASYNC_RESULT*>(lpvStatusInformation);
@@ -222,6 +293,14 @@ class WinInetRequestWrapper
         }
     }
 
+    void DispatchEvent(HttpStateEvent type)
+    {
+        if (m_appCallback != nullptr)
+        {
+            m_appCallback->OnHttpStateEvent(type, static_cast<void*>(m_hWinInetRequest), 0);
+        }
+    }
+
     void onRequestComplete(DWORD dwError)
     {
         std::unique_ptr<SimpleHttpResponse> response(new SimpleHttpResponse(m_id));
@@ -257,6 +336,7 @@ class WinInetRequestWrapper
             } while (m_bufferUsed == sizeof(m_buffer));
         }
 
+        // SUCCESS with no IO_PENDING means we're done with the response body: try to parse the response headers.
         if (dwError == ERROR_SUCCESS) {
             response->m_result = HttpResult_OK;
 
@@ -308,7 +388,10 @@ class WinInetRequestWrapper
                 response->m_headers.add(name, value1);
                 ptr = eol + 2;
             }
-
+            // This event handler covers the only positive case when we actually got some server response.
+            // We may still invoke OnHttpResponse(...) below for this positive as well as other negative
+            // cases where there was a short-read, connection failuire or timeout on reading the response.
+            DispatchEvent(OnResponse);
         } else {
             switch (dwError) {
                 case ERROR_INTERNET_OPERATION_CANCELLED:
@@ -348,9 +431,18 @@ class WinInetRequestWrapper
             }
         }
 
-        // 'response' gets released in EventsUploadContext.clear()
-        m_appCallback->OnHttpResponse(response.release());
-        m_parent.erase(m_id);
+        assert(isCallbackCalled==false);
+        if (!isCallbackCalled)
+        {
+            // Only one WinInet worker thread may invoke async callback for a given request at any given moment of time.
+            // That ensures that isCallbackCalled does not require a lock around it. We unregister the callback here
+            // to ensure that no more callbacks are coming for that m_hWinInetRequest.
+            ::InternetSetStatusCallback(m_hWinInetRequest, NULL);
+            isCallbackCalled = true;
+            m_appCallback->OnHttpResponse(response.release());
+            // HttpClient parent is destroying this HttpRequest object by id
+            m_parent.erase(m_id);
+        }
     }
 };
 
@@ -454,3 +546,4 @@ bool HttpClient_WinInet::IsMsRootCheckRequired()
 } ARIASDK_NS_END
 
 #endif // HAVE_MAT_DEFAULT_HTTP_CLIENT
+// clang-format on

lib/include/public/ITaskDispatcher.hpp

@@ -40,7 +40,11 @@ namespace ARIASDK_NS_BEGIN
             /// <summary>
             /// A Done item is an item that has been marked by the worker thread as already completed.
             /// </summary>
-            Done
+            Done,
+            /// <summary>
+            /// A Cancelled item is an item that has been marked by the worker thread as Cancelled.
+            /// </summary>
+            Cancelled
         } Type;
 
         /// <summary>
@@ -92,8 +96,9 @@ namespace ARIASDK_NS_BEGIN
         /// Cancel a previously queued tasks
         /// </summary>
         /// <param name="task">Task to be cancelled</param>
+        /// <param name="waitTime">Amount of time to wait for if the task is currently executing</param>
         /// <returns>True if successfully cancelled, else false</returns>
-        virtual bool Cancel(Task* task) = 0;
+        virtual bool Cancel(Task* task, uint64_t waitTime = 0) = 0;
     };
 
     /// @endcond