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

build-tests.cmd

@@ -53,12 +53,10 @@ set MAXCPUCOUNT=%NUMBER_OF_PROCESSORS%
 set SOLUTION=Solutions\MSTelemetrySDK.sln
 
 msbuild %SOLUTION% /target:sqlite:Rebuild,zlib:Rebuild,Tests\gmock:Rebuild,Tests\gtest:Rebuild,Tests\UnitTests:Rebuild,Tests\FuncTests:Rebuild /p:BuildProjectReferences=true /maxcpucount:%MAXCPUCOUNT% /detailedsummary /p:Configuration=%CONFIGURATION% /p:Platform=%PLAT% %CUSTOM_PROPS%
-if errorLevel 1 goto end
+if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL%
 Solutions\out\%CONFIGURATION%\%PLAT%\UnitTests\UnitTests.exe
-if errorLevel 1 goto end
+if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL%
 Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe
-:end
-if errorLevel 1 goto end
-start "" Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe --gtest_filter=MultipleLogManagersTests.MultiProcessesLogManager
-start "" Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe --gtest_filter=MultipleLogManagersTests.MultiProcessesLogManager
-:end
+if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL%
+powershell -NoProfile -ExecutionPolicy Bypass -Command "$path = Join-Path (Get-Location) 'Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe'; $args = '--gtest_filter=MultipleLogManagersTests.MultiProcessesLogManager'; $p1 = Start-Process -FilePath $path -ArgumentList $args -PassThru; $p2 = Start-Process -FilePath $path -ArgumentList $args -PassThru; $p1.WaitForExit(); $p2.WaitForExit(); if ($p1.ExitCode -ne 0 -or $p2.ExitCode -ne 0) { exit 1 }"
+if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL%

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,90 @@ 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()
+    void HttpClientManager::cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout)
     {
+        if (bestEffortTimeout > std::chrono::milliseconds::zero())
+        {
+#if defined(_CPPRTTI) || defined(__GXX_RTTI)
+            auto boundedCancel = dynamic_cast<IBoundedHttpClientCancel*>(&m_httpClient);
+            if (boundedCancel != nullptr)
+            {
+                boundedCancel->CancelAllRequests(bestEffortTimeout);
+                return;
+            }
+#endif
+
+            cancelTrackedRequestsAsync();
+            return;
+        }
+
         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)
                 {
-                    return;
+                    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())
+                {
+                    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)
+    {
+        // Use the transport-specific bounded path when available; older clients
+        // fall back to cancelling tracked requests individually.
+        const auto cancelStart = std::chrono::steady_clock::now();
+        cancelAllRequestsAsync(bestEffort ? m_cancelDrainTimeout : std::chrono::milliseconds::zero());
+
+        // Drain callbacks through the condition variable signaled by onHttpResponse.
+        std::unique_lock<std::recursive_mutex> lock(m_httpCallbacksMtx);
+        if (bestEffort)
+        {
+            // Keep pause bounded, including time spent in the transport cancel.
+            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 is the lifetime barrier for callback state, so drain fully.
+            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,9 @@ class HttpClientManager
 
         virtual ~HttpClientManager() noexcept;
 
-        void cancelAllRequests();
+        // Cancel in-flight requests. Shutdown drains fully; pause uses a bounded,
+        // best-effort drain because it may run under the LogManager lock.
+        void cancelAllRequests(bool bestEffort = false);
 
         size_t requestCount() const
         {
@@ -55,14 +59,22 @@ class HttpClientManager
         void handleSendRequest(EventsUploadContextPtr const& ctx);
         virtual void scheduleOnHttpResponse(HttpCallback* callback);
         void onHttpResponse(HttpCallback* callback);
-        bool cancelAllRequestsAsync();
+        void 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

@@ -503,6 +503,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;
     }
@@ -535,6 +537,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;
@@ -548,11 +555,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

@@ -399,6 +399,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;
@@ -412,11 +417,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
-

lib/http/IBoundedHttpClientCancel.hpp

@@ -0,0 +1,24 @@
+//
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+//
+
+#pragma once
+
+#include "ctmacros.hpp"
+
+#include <chrono>
+
+namespace MAT_NS_BEGIN {
+
+class IBoundedHttpClientCancel
+{
+public:
+    virtual ~IBoundedHttpClientCancel() noexcept = default;
+
+    // Positive timeout is a best-effort cap. Zero means the caller requires a
+    // full drain, matching IHttpClient::CancelAllRequests().
+    virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) = 0;
+};
+
+} MAT_NS_END