Merge remote-tracking branch 'origin/bhamehta/winhttp-default-windows-transport' into bhamehta/winhttp-default-windows-transport

# Conflicts:
#	lib/http/HttpClientManager.cpp
#	lib/http/HttpClientManager.hpp
#	lib/system/TelemetrySystem.cpp

Author: bmehta001

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

@@ -139,6 +139,7 @@ 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();
         }
 
@@ -197,12 +198,16 @@ namespace MAT_NS_BEGIN {
 
     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)
@@ -211,11 +216,12 @@ namespace MAT_NS_BEGIN {
                     [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()));
+                         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

@@ -30,6 +30,8 @@ class HttpClientManager
 
         virtual ~HttpClientManager() noexcept;
 
+        // 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
@@ -65,7 +67,13 @@ class HttpClientManager
         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)};
 };
 

lib/http/HttpClient_WinInet.cpp

@@ -502,6 +502,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;
     }
@@ -534,6 +536,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;
@@ -547,11 +554,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

@@ -396,6 +396,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;
@@ -409,11 +414,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/include/public/IHttpClient.hpp

@@ -556,6 +556,10 @@ namespace MAT_NS_BEGIN
         /// <param name="id">A string that contains the ID of the request to cancel.</param>
         virtual void CancelRequestAsync(std::string const& id) = 0;
 
+        /// <summary>
+        /// Cancels all pending requests, draining fully before returning when the
+        /// implementation owns a synchronous drain.
+        /// </summary>
         virtual void CancelAllRequests() {}
 
         /// <summary>
@@ -572,4 +576,3 @@ namespace MAT_NS_BEGIN
 } MAT_NS_END
 
 #endif
-

lib/jni/JniConvertors.cpp

@@ -13,6 +13,11 @@ std::string JStringToStdString(JNIEnv* env, const jstring& jstr) {
 
     size_t jstr_length = env->GetStringUTFLength(jstr);
     auto jstr_utf = env->GetStringUTFChars(jstr, nullptr);
+    if (jstr_utf == nullptr) {
+        // Preserve the pending Java exception (typically an allocation failure)
+        // so the JNI caller observes the real failure instead of an empty value.
+        return "";
+    }
     std::string str(jstr_utf, jstr_utf + jstr_length);
     env->ReleaseStringUTFChars(jstr, jstr_utf);
     return str;
@@ -160,7 +165,7 @@ EventProperties GetEventProperties(JNIEnv* env, const jstring& jstrEventName, co
     EventProperties eventProperties;
     eventProperties.SetName(JStringToStdString(env, jstrEventName));
     if (jstrEventType != NULL) {
-        // An empty type means "unset" (the native default). Before #1329 the
+        // An empty type means "unset" (the native default). Previously the
         // Java getType() returned null for a default EventProperties, so this
         // branch was skipped. getType() now returns "" to fix a Java-side NPE;
         // forwarding SetType("") here would fail native event-name validation
@@ -208,4 +213,3 @@ std::vector<std::string> ConvertJObjectArrayToStdStringVector(JNIEnv* env, const
 }
 
 } MAT_NS_END
-

lib/jni/Signals_jni.cpp

@@ -25,11 +25,22 @@ Java_com_microsoft_applications_events_Signals_sendSignal(JNIEnv *env,
                                                           jlong nativeLoggerPtr,
                                                           jstring signal_item_json) {
     jboolean isCopy = true;
+    auto logger = reinterpret_cast<ILogger*>(nativeLoggerPtr);
+    if (logger == nullptr) {
+        return false;
+    }
+    if (signal_item_json == nullptr) {
+        return false;
+    }
     const char *signalItemJson = (env)->GetStringUTFChars(signal_item_json, &isCopy);
-    env->ReleaseStringUTFChars(signal_item_json, signalItemJson);
+    if (signalItemJson == nullptr) {
+        // Preserve the pending Java exception (typically an allocation failure)
+        // rather than masking it as a clean false result.
+        return false;
+    }
 
-    auto logger = reinterpret_cast<ILogger*>(nativeLoggerPtr);
     EventProperties eventProperties = Signals::CreateEventProperties(signalItemJson);
+    env->ReleaseStringUTFChars(signal_item_json, signalItemJson);
     logger->LogEvent(eventProperties);
     return true;
 }
@@ -54,20 +65,34 @@ Java_com_microsoft_applications_events_Signals_nativeInitialize(JNIEnv *env, jcl
     SubstrateSignalsConfiguration config;
 
     jboolean isCopy = true;
-    const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy);
-    if (strlen(convertedValue) > 0) {
-        config.ServiceRequestConfig.BaseUrl = convertedValue;
+    if (base_url != nullptr) {
+        const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy);
+        if (convertedValue == nullptr) {
+            // Preserve the pending Java exception (typically an allocation failure)
+            // rather than masking it as a clean false result.
+            return false;
+        }
+        if (strlen(convertedValue) > 0) {
+            config.ServiceRequestConfig.BaseUrl = convertedValue;
+        }
+        env->ReleaseStringUTFChars(base_url, convertedValue);
     }
-    env->ReleaseStringUTFChars(base_url, convertedValue);
 
     config.ServiceRequestConfig.TimeoutMs = reinterpret_cast<int>(timeout_ms);
     config.ServiceRequestConfig.RetryTimes = reinterpret_cast<int>(retry_times);
     config.ServiceRequestConfig.RetryTimesToWait = reinterpret_cast<int>(retry_time_to_wait);
 
-    jsize size = env->GetArrayLength(retry_status_codes);
-    std::vector<int> retryStatusCodes(size);
-    env->GetIntArrayRegion(retry_status_codes, jsize{0}, size, &retryStatusCodes[0] );
-    config.ServiceRequestConfig.RetryStatusCodes = std::vector<int64_t>(retryStatusCodes.begin(), retryStatusCodes.end());
+    if (retry_status_codes != nullptr)
+    {
+        jsize size = env->GetArrayLength(retry_status_codes);
+        std::vector<int> retryStatusCodes(size);
+        if (size > 0)
+        {
+            env->GetIntArrayRegion(retry_status_codes, jsize{0}, size, retryStatusCodes.data());
+        }
+        config.ServiceRequestConfig.RetryStatusCodes =
+            std::vector<int64_t>(retryStatusCodes.begin(), retryStatusCodes.end());
+    }
 
     spDataInspector = Signals::CreateSignalsEventInspector(nullptr, config);
     return true;