Fix JNI string lifetime/null-safety, Windows cancel-drain race, and UWP version parse

lib/http/HttpClientManager.cpp

@@ -4,13 +4,15 @@
 //
 
 #include "HttpClientManager.hpp"
+#include "IBoundedHttpClientCancel.hpp"
 #include "utils/StringUtils.hpp"
 #include "pal/TaskDispatcher.hpp"
 
 #include <assert.h>
 #include <algorithm>
 #include <chrono>
 #include <thread>
+#include <vector>
 
 #ifdef linux
 #include <unistd.h>
@@ -137,34 +139,114 @@ namespace MAT_NS_BEGIN {
 
             LOG_TRACE("HTTP remove callback=%p", callback);
             m_httpCallbacks.remove(callback);
+            // Wake cancelAllRequests() waiting for the list to drain.
+            m_httpCallbacksCV.notify_all();
         }
 
         delete callback;
     }
 
-    bool HttpClientManager::cancelAllRequestsAsync()
+    bool HttpClientManager::cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout)
     {
+        if (bestEffortTimeout > std::chrono::milliseconds::zero())
+        {
+            auto boundedCancel = dynamic_cast<IBoundedHttpClientCancel*>(&m_httpClient);
+            if (boundedCancel != nullptr)
+            {
+                boundedCancel->CancelAllRequests(bestEffortTimeout);
+                return true;
+            }
+
+            cancelTrackedRequestsAsync();
+            return false;
+        }
+
         m_httpClient.CancelAllRequests();
         return true;
     }
 
-    void HttpClientManager::cancelAllRequests()
+    void HttpClientManager::cancelTrackedRequestsAsync()
     {
-        cancelAllRequestsAsync();
-
-        // Wait for callbacks to drain before shutdown can destroy state that
-        // those callbacks still use. Keep the list check synchronized and sleep
-        // between polls so a slow adapter does not burn CPU while draining.
-        for (;;)
+        std::vector<std::string> requestIds;
         {
+            LOCKGUARD(m_httpCallbacksMtx);
+            for (const auto& callback : m_httpCallbacks)
             {
-                LOCKGUARD(m_httpCallbacksMtx);
-                if (m_httpCallbacks.empty())
+                if (callback == nullptr || callback->m_ctx == nullptr)
+                {
+                    continue;
+                }
+
+                std::string id = callback->m_ctx->httpRequestId;
+                if (id.empty() && callback->m_ctx->httpRequest != nullptr)
+                {
+                    id = callback->m_ctx->httpRequest->GetId();
+                }
+                if (!id.empty())
                 {
-                    return;
+                    requestIds.push_back(id);
                 }
             }
-            std::this_thread::sleep_for(std::chrono::milliseconds(50));
+        }
+
+        for (const auto& id : requestIds)
+        {
+            m_httpClient.CancelRequestAsync(id);
+        }
+    }
+
+    void HttpClientManager::cancelAllRequests(bool bestEffort)
+    {
+        // On clients that opt into bounded cancellation (notably synchronous-response
+        // Windows transports), the transport's CancelAllRequests() is where
+        // cancellation actually blocks, so pass the best-effort deadline down to
+        // bound it. Older clients fall back to per-request async cancel; the
+        // manager-level drain below is the bound for those async-handler paths. A
+        // zero timeout means "drain fully".
+        const auto cancelStart = std::chrono::steady_clock::now();
+        cancelAllRequestsAsync(bestEffort ? m_cancelDrainTimeout : std::chrono::milliseconds::zero());
+
+        // Drain in-flight callbacks via a condition variable signaled from
+        // onHttpResponse -- never a busy/poll loop, which burned 100% CPU while
+        // holding the LogManager lock.
+        std::unique_lock<std::recursive_mutex> lock(m_httpCallbacksMtx);
+        if (bestEffort)
+        {
+            // Pause (and similar) must not block the caller indefinitely -- it may
+            // hold the LogManager lock (the observed spindump was PauseTransmission).
+            // The manager is NOT being destroyed here, so outstanding callbacks remain
+            // valid and drain later; cap the wait as a safety valve.
+            //
+            // This bounds the drain of m_httpCallbacks, which is the blocking region on
+            // the async-handler and old-style-client fallback paths. On Windows
+            // (USE_SYNC_HTTPRESPONSE_HANDLER), bounded-capability clients drain
+            // m_httpCallbacks synchronously inside the transport cancel call above, so
+            // this wait is usually already satisfied; the real blocking there is the
+            // transport-level wait (WinInet condition-variable wait, WinRt poll), which
+            // is bounded by the same best-effort deadline passed into the capability.
+            //
+            // Treat m_cancelDrainTimeout as the total budget for the pause: subtract the
+            // time already spent in the transport cancel so the whole path holds the
+            // LogManager lock for at most ~m_cancelDrainTimeout, not up to 2x it.
+            const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
+                std::chrono::steady_clock::now() - cancelStart);
+            const auto remaining = (elapsed < m_cancelDrainTimeout)
+                ? (m_cancelDrainTimeout - elapsed) : std::chrono::milliseconds::zero();
+            if (!m_httpCallbacksCV.wait_for(lock, remaining,
+                    [this] { return m_httpCallbacks.empty(); }))
+            {
+                LOG_WARN("cancelAllRequests: %zu callback(s) still draining after %lld ms (best-effort)",
+                         m_httpCallbacks.size(), static_cast<long long>(m_cancelDrainTimeout.count()));
+            }
+        }
+        else
+        {
+            // Shutdown/cleanup: this is the lifetime barrier before state the
+            // callbacks reference is destroyed, so drain fully. The CV keeps it
+            // efficient (no CPU spin) and it returns as soon as the last callback is
+            // handled -- a stalled drain here indicates the caller stopped the task
+            // dispatcher before cancelling, which it must not do.
+            m_httpCallbacksCV.wait(lock, [this] { return m_httpCallbacks.empty(); });
         }
     }
 

lib/http/HttpClientManager.hpp

@@ -12,6 +12,8 @@
 
 #include <list>
 #include <mutex>
+#include <chrono>
+#include <condition_variable>
 
 namespace MAT_NS_BEGIN
 {
@@ -28,7 +30,13 @@ class HttpClientManager
 
         virtual ~HttpClientManager() noexcept;
 
-        void cancelAllRequests();
+        // Cancel in-flight requests and drain their callbacks. bestEffort=false (the
+        // default, used by shutdown/cleanup) drains fully -- it is the lifetime
+        // barrier before state the callbacks reference is destroyed. bestEffort=true
+        // (used by pause) caps the wait so it cannot block a caller that holds the
+        // LogManager lock; the manager is not being destroyed, so outstanding
+        // callbacks stay valid and drain later.
+        void cancelAllRequests(bool bestEffort = false);
 
         size_t requestCount() const
         {
@@ -55,14 +63,22 @@ class HttpClientManager
         void handleSendRequest(EventsUploadContextPtr const& ctx);
         virtual void scheduleOnHttpResponse(HttpCallback* callback);
         void onHttpResponse(HttpCallback* callback);
-        bool cancelAllRequestsAsync();
+        bool cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout = std::chrono::milliseconds::zero());
+        void cancelTrackedRequestsAsync();
 
         ILogManager&              m_logManager;
         IHttpClient&              m_httpClient;
         ITaskDispatcher&          m_taskDispatcher;
         mutable std::recursive_mutex m_httpCallbacksMtx;
         std::list<HttpCallback*>  m_httpCallbacks;
+        // Signaled from onHttpResponse when a callback is removed, so cancelAllRequests
+        // can drain via a condition variable instead of a poll loop.
+        std::condition_variable_any m_httpCallbacksCV;
+        // Upper bound on how long cancelAllRequests waits for callbacks to drain. A
+        // last-resort safety valve so a stalled dispatcher/HTTP stack can never make
+        // the drain spin or block forever. Adjustable so tests can
+        // exercise the timeout path without a long wait.
+        std::chrono::milliseconds m_cancelDrainTimeout{std::chrono::seconds(30)};
 };
 
 } MAT_NS_END
-

lib/http/HttpClient_WinInet.cpp

@@ -489,6 +489,8 @@ void HttpClient_WinInet::erase(std::string const& id)
     if (it != m_requests.end()) {
         auto req = it->second;
         m_requests.erase(it);
+        // Wake CancelAllRequests() waiting for the map to drain.
+        m_requestsCV.notify_all();
         // delete WinInetRequestWrapper
         delete req;
     }
@@ -521,6 +523,11 @@ void HttpClient_WinInet::CancelRequestAsync(std::string const& id)
 
 
 void HttpClient_WinInet::CancelAllRequests()
+{
+    CancelAllRequests(std::chrono::milliseconds::zero());
+}
+
+void HttpClient_WinInet::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout)
 {
     // vector of all request IDs
     std::vector<std::string> ids;
@@ -534,11 +541,26 @@ void HttpClient_WinInet::CancelAllRequests()
     for (const auto &id : ids)
         CancelRequestAsync(id);
 
-    // wait for all destructors to run
-    while (!m_requests.empty())
+    // Wait for all request destructors to run (erase() removes them on the WinInet
+    // callback thread). Use a condition variable signaled from erase() rather than a
+    // poll loop so this never spins at 100% CPU while draining. WinInet delivers the
+    // cancellation callbacks on its own threads, so the wait completes without
+    // depending on the SDK task dispatcher.
+    std::unique_lock<std::recursive_mutex> lock(m_requestsMutex);
+    if (bestEffortTimeout > std::chrono::milliseconds::zero())
+    {
+        // Best-effort (e.g. pause): the caller must not block indefinitely. The client
+        // is NOT being destroyed here, so a late callback that arrives after this
+        // returns still runs erase() on a live client -- returning early is safe.
+        m_requestsCV.wait_for(lock, bestEffortTimeout, [this] { return m_requests.empty(); });
+    }
+    else
     {
-        PAL::sleep(100);
-        std::this_thread::yield();
+        // Full drain barrier (the destructor calls this): returning early with
+        // requests still in flight would let a late WinInet callback invoke
+        // WinInetRequestWrapper::OnHttpResponse -> m_parent.erase() on a destroyed
+        // client, so wait for every request to drain.
+        m_requestsCV.wait(lock, [this] { return m_requests.empty(); });
     }
 }
 

lib/http/HttpClient_WinInet.hpp

@@ -8,10 +8,14 @@
 #ifdef HAVE_MAT_DEFAULT_HTTP_CLIENT
 
 #include "IHttpClient.hpp"
+#include "IBoundedHttpClientCancel.hpp"
 #include "pal/PAL.hpp"
 
 #include "ILogManager.hpp"
 
+#include <condition_variable>
+#include <mutex>
+
 namespace MAT_NS_BEGIN {
 
 #ifndef _WININET_
@@ -20,7 +24,7 @@ typedef void* HINTERNET;
 
 class WinInetRequestWrapper;
 
-class HttpClient_WinInet : public IHttpClient {
+class HttpClient_WinInet : public IHttpClient, public IBoundedHttpClientCancel {
   public:
     // Common IHttpClient methods
     HttpClient_WinInet();
@@ -29,6 +33,7 @@ class HttpClient_WinInet : public IHttpClient {
     virtual void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) final;
     virtual void CancelRequestAsync(std::string const& id) final;
     virtual void CancelAllRequests() final;
+    virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) final;
 
     virtual void ApplySettings(ILogConfiguration& config) override;
 
@@ -43,6 +48,9 @@ class HttpClient_WinInet : public IHttpClient {
     HINTERNET                                                        m_hInternet;
     std::recursive_mutex                                             m_requestsMutex;
     std::map<std::string, WinInetRequestWrapper*>                    m_requests;
+    // Signaled from erase() when a request is removed, so CancelAllRequests can drain
+    // via a condition variable instead of a poll loop (no 100% CPU spin).
+    std::condition_variable_any                                      m_requestsCV;
     static unsigned                                                  s_nextRequestId;
     bool                                                             m_msRootCheck;
     friend class WinInetRequestWrapper;
@@ -53,4 +61,3 @@ class HttpClient_WinInet : public IHttpClient {
 #endif // HAVE_MAT_DEFAULT_HTTP_CLIENT
 
 #endif // HTTPCLIENT_WININET_HPP
-

lib/http/HttpClient_WinRt.cpp

@@ -338,6 +338,11 @@ namespace MAT_NS_BEGIN {
     }
 
     void HttpClient_WinRt::CancelAllRequests()
+    {
+        CancelAllRequests(std::chrono::milliseconds::zero());
+    }
+
+    void HttpClient_WinRt::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout)
     {
         // vector of all request IDs
         std::vector<std::string> ids;
@@ -351,11 +356,40 @@ namespace MAT_NS_BEGIN {
         for (const auto &id : ids)
             CancelRequestAsync(id);
 
-        // wait for all destructors to run
-        while (!m_requests.empty())
+        // wait for all destructors to run. Read m_requests under the lock each
+        // iteration; erase() runs on the PPL continuation thread under the same lock.
+        // A zero timeout drains fully (shutdown); a positive timeout is a best-effort
+        // cap so callers such as pause do not block indefinitely.
+        const bool bounded = bestEffortTimeout > std::chrono::milliseconds::zero();
+        const auto deadline = std::chrono::steady_clock::now() + bestEffortTimeout;
+        bool done;
         {
-            PAL::sleep(100);
+            std::lock_guard<std::mutex> lock(m_requestsMutex);
+            done = m_requests.empty();
+        }
+        while (!done)
+        {
+            if (bounded)
+            {
+                const auto now = std::chrono::steady_clock::now();
+                if (now >= deadline)
+                    break;
+                // Sleep no longer than the remaining budget so the bounded wait does not
+                // overshoot bestEffortTimeout by up to a full poll interval.
+                long long remainingMs = std::chrono::duration_cast<std::chrono::milliseconds>(deadline - now).count();
+                if (remainingMs < 1) remainingMs = 1;
+                if (remainingMs > 100) remainingMs = 100;
+                PAL::sleep(static_cast<unsigned>(remainingMs));
+            }
+            else
+            {
+                PAL::sleep(100);
+            }
             std::this_thread::yield();
+            {
+                std::lock_guard<std::mutex> lock(m_requestsMutex);
+                done = m_requests.empty();
+            }
         }
     };
 

lib/http/HttpClient_WinRt.hpp

@@ -13,6 +13,7 @@
 #include <Windows.h>
 
 #include "IHttpClient.hpp"
+#include "IBoundedHttpClientCancel.hpp"
 #include "pal/PAL.hpp"
 
 #include <ppltasks.h>
@@ -28,14 +29,15 @@ namespace MAT_NS_BEGIN {
 
 class WinRtRequestWrapper;
 
-class HttpClient_WinRt : public IHttpClient {
+class HttpClient_WinRt : public IHttpClient, public IBoundedHttpClientCancel {
   public:
     HttpClient_WinRt();
     virtual ~HttpClient_WinRt();
     virtual IHttpRequest* CreateRequest() override;
     virtual void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) override;
     virtual void CancelRequestAsync(std::string const& id) override;
     virtual void CancelAllRequests() override;
+    virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) override;
     HttpClient^ getHttpClient() { return m_httpClient; }
 
   protected:
@@ -55,4 +57,3 @@ class HttpClient_WinRt : public IHttpClient {
 #endif // HAVE_MAT_DEFAULT_HTTP_CLIENT
 
 #endif // HTTPCLIENT_WINRT_HPP
-