From cd2f1a954b0c944b0708d50312dc8a5d12de645f Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 11:52:58 -0500 Subject: [PATCH 01/20] Fix four latent safety bugs (JNI lifetime/null-safety, cancel race, UWP parse) Bundled small fixes from a repo-wide review. None are recent regressions (introduced 2018-2023). 1) Signals_jni.cpp (UAF): sendSignal called ReleaseStringUTFChars on the UTF buffer *before* passing it to Signals::CreateEventProperties (which copies from it by value), reading freed/unpinned memory. Move the release to after the buffer is consumed. 2) HttpClient_WinInet.cpp / HttpClient_WinRt.cpp (data race): the CancelAllRequests() drain loop read `m_requests.empty()` with no lock held, while erase() mutates the map on the HTTP callback / PPL continuation thread under m_requestsMutex -- a data race / UB on std::map. Read empty() under the lock each iteration, mirroring the already-correct WinRt destructor drain. (The recent #1460 fixed the destructor's drain but not these methods.) 3) JniConvertors.cpp / OfflineStorage_Room.cpp (null deref): the result of GetStringUTFChars (which returns null on allocation failure) was fed straight into a std::string ctor. Null-check before constructing. 4) WindowsRuntimeSystemInformationImpl.cpp (UWP): std::stoull on the DeviceFamilyVersion string was unguarded; guard it with try/catch defaulting to 0 (the code already has a versionDec==0 -> "10.0" fallback). Validation: JniConvertors.cpp and OfflineStorage_Room.cpp pass NDK aarch64 -fsyntax-only. Signals_jni (needs the private signals module), the Windows HTTP clients, and the UWP path can't be built on this host and rely on the Android/Windows CI; the WinInet/WinRt fix mirrors the existing correct destructor pattern in the same files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_WinInet.cpp | 14 ++++++++++++-- lib/http/HttpClient_WinRt.cpp | 14 ++++++++++++-- lib/jni/JniConvertors.cpp | 2 ++ lib/jni/Signals_jni.cpp | 2 +- lib/offline/OfflineStorage_Room.cpp | 5 +++-- .../WindowsRuntimeSystemInformationImpl.cpp | 10 +++++++++- 6 files changed, 39 insertions(+), 8 deletions(-) diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index eaefb2318..9c920682d 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -534,11 +534,21 @@ void HttpClient_WinInet::CancelAllRequests() 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 WinInet callback thread under the same lock. + bool done; + { + LOCKGUARD(m_requestsMutex); + done = m_requests.empty(); + } + while (!done) { PAL::sleep(100); std::this_thread::yield(); + { + LOCKGUARD(m_requestsMutex); + done = m_requests.empty(); + } } } diff --git a/lib/http/HttpClient_WinRt.cpp b/lib/http/HttpClient_WinRt.cpp index e46e4c49c..db14aa1da 100644 --- a/lib/http/HttpClient_WinRt.cpp +++ b/lib/http/HttpClient_WinRt.cpp @@ -351,11 +351,21 @@ 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. + bool done; + { + std::lock_guard lock(m_requestsMutex); + done = m_requests.empty(); + } + while (!done) { PAL::sleep(100); std::this_thread::yield(); + { + std::lock_guard lock(m_requestsMutex); + done = m_requests.empty(); + } } }; diff --git a/lib/jni/JniConvertors.cpp b/lib/jni/JniConvertors.cpp index 87b7cb15b..61eceafff 100644 --- a/lib/jni/JniConvertors.cpp +++ b/lib/jni/JniConvertors.cpp @@ -13,6 +13,8 @@ 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) + return ""; std::string str(jstr_utf, jstr_utf + jstr_length); env->ReleaseStringUTFChars(jstr, jstr_utf); return str; diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 59ca6f32d..8b2ef0111 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -26,10 +26,10 @@ Java_com_microsoft_applications_events_Signals_sendSignal(JNIEnv *env, jstring signal_item_json) { jboolean isCopy = true; const char *signalItemJson = (env)->GetStringUTFChars(signal_item_json, &isCopy); - env->ReleaseStringUTFChars(signal_item_json, signalItemJson); auto logger = reinterpret_cast(nativeLoggerPtr); EventProperties eventProperties = Signals::CreateEventProperties(signalItemJson); + env->ReleaseStringUTFChars(signal_item_json, signalItemJson); logger->LogEvent(eventProperties); return true; } diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index 72a04d0ed..c3b61d989 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -722,8 +722,9 @@ namespace MAT_NS_BEGIN auto count = env->GetLongField(byTenant, count_id); ThrowLogic(env, "Exception fetching count"); auto utf = env->GetStringUTFChars(token, nullptr); - std::string key(utf); - env->ReleaseStringUTFChars(token, utf); + std::string key(utf != nullptr ? utf : ""); + if (utf != nullptr) + env->ReleaseStringUTFChars(token, utf); dropped[key] = static_cast(count); env.popLocalFrame(); } diff --git a/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp b/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp index 2ae7e9af4..8c4ff7f66 100644 --- a/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp +++ b/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp @@ -80,7 +80,15 @@ namespace PAL_NS_BEGIN { // The DeviceFamilyVersion is a decimalized form of the ULONGLONG hex form. For example: // 2814750430068736 = 000A000027840000 = 10.0.10116.0 - auto versionDec = std::stoull(AnalyticsInfo::VersionInfo->DeviceFamilyVersion->Data()); + unsigned long long versionDec = 0ull; + try + { + versionDec = std::stoull(AnalyticsInfo::VersionInfo->DeviceFamilyVersion->Data()); + } + catch (const std::exception&) + { + versionDec = 0ull; + } if (versionDec != 0ull) { m_os_major_version = std::to_string(versionDec >> 16 * 3) + "." + std::to_string(versionDec >> 16 * 2 & 0xFFFF); From 4c176db78b026fbeff41a6e39be94faced8a8cd6 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 16:00:31 -0500 Subject: [PATCH 02/20] Address review comments: harden two JNI string reads lib/jni/Signals_jni.cpp (sendSignal): guard the jstring argument and the GetStringUTFChars() result for null before use. A null return (e.g. OOM, with a pending exception) previously flowed into CreateEventProperties()/Release as a null pointer. Now returns false instead. lib/offline/OfflineStorage_Room.cpp (ReleaseRecords): a failed tenant-token string read was turned into an empty std::string and reported as dropped[""] = count, silently misattributing dropped records to an empty tenant token. Now follows the file's established pattern (GetStringUTFChars + ThrowRuntime) and skips the entry when the read fails instead of fabricating an empty token. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/Signals_jni.cpp | 8 ++++++++ lib/offline/OfflineStorage_Room.cpp | 9 +++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 8b2ef0111..6de25c4fb 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -25,7 +25,15 @@ Java_com_microsoft_applications_events_Signals_sendSignal(JNIEnv *env, jlong nativeLoggerPtr, jstring signal_item_json) { jboolean isCopy = true; + if (signal_item_json == nullptr) { + return false; + } const char *signalItemJson = (env)->GetStringUTFChars(signal_item_json, &isCopy); + if (signalItemJson == nullptr) { + // GetStringUTFChars returned null (e.g. OOM, with a pending exception); + // there is nothing valid to log. + return false; + } auto logger = reinterpret_cast(nativeLoggerPtr); EventProperties eventProperties = Signals::CreateEventProperties(signalItemJson); diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index c3b61d989..ef4f90c63 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -722,10 +722,15 @@ namespace MAT_NS_BEGIN auto count = env->GetLongField(byTenant, count_id); ThrowLogic(env, "Exception fetching count"); auto utf = env->GetStringUTFChars(token, nullptr); - std::string key(utf != nullptr ? utf : ""); + ThrowRuntime(env, "Exception fetching token string"); + // Skip rather than misattribute dropped records to an empty + // tenant token when the string read fails. if (utf != nullptr) + { + std::string key(utf); env->ReleaseStringUTFChars(token, utf); - dropped[key] = static_cast(count); + dropped[key] = static_cast(count); + } env.popLocalFrame(); } m_observer->OnStorageRecordsDropped(dropped); From dca30a733ba7a559fa97317e737be6ea74523afb Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 24 Jun 2026 11:58:10 -0500 Subject: [PATCH 03/20] Guard all remaining GetStringUTFChars reads against null Addresses @lalitb's review: Signals_jni nativeInitialize called strlen(convertedValue) (and ReleaseStringUTFChars) on the result of GetStringUTFChars(base_url, ...) with no null check, so a failed conversion (e.g. pending OOM) would dereference null. Guard it: only read/release when non-null; an empty/failed base_url keeps the existing default (BaseUrl left unset). While here, make the JNI string null-safety consistent across the file this PR already hardens. ThrowRuntime/ThrowLogic only throw when s_throwExceptions is true; otherwise they ExceptionClear() and execution continues, so the existing checks are not sufficient on their own. The other GetStringUTFChars sites in OfflineStorage_Room.cpp still used the pointer unguarded: - GetReservedRecords: token_utf passed straight into StorageRecord and ReleaseStringUTFChars (null -> std::string(nullptr) UB / release of null). - GetSetting: result = utf with no null check. - GetRecords: tenant_utf passed into emplace_back / released unguarded. Each now mirrors the guard already added for the dropped-records path: substitute an empty token (matching JniConvertors' canonical return "" on null) and only release when non-null. This avoids null deref and release-of-null without silently changing behavior on success. Validated with NDK r29 clang -fsyntax-only (aarch64-linux-android23, -std=c++14, JNI build defines) on both files: clean. Files changed: - lib/jni/Signals_jni.cpp: null-guard base_url GetStringUTFChars - lib/offline/OfflineStorage_Room.cpp: null-guard token_utf, result, tenant_utf Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/Signals_jni.cpp | 8 +++++--- lib/offline/OfflineStorage_Room.cpp | 21 +++++++++++++++------ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 6de25c4fb..918a84774 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -63,10 +63,12 @@ Java_com_microsoft_applications_events_Signals_nativeInitialize(JNIEnv *env, jcl jboolean isCopy = true; const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy); - if (strlen(convertedValue) > 0) { - config.ServiceRequestConfig.BaseUrl = convertedValue; + if (convertedValue != nullptr) { + if (strlen(convertedValue) > 0) { + config.ServiceRequestConfig.BaseUrl = convertedValue; + } + env->ReleaseStringUTFChars(base_url, convertedValue); } - env->ReleaseStringUTFChars(base_url, convertedValue); config.ServiceRequestConfig.TimeoutMs = reinterpret_cast(timeout_ms); config.ServiceRequestConfig.RetryTimes = reinterpret_cast(retry_times); diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index ef4f90c63..dbe68ea1a 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -498,14 +498,17 @@ namespace MAT_NS_BEGIN uint8_t* end = start + env->GetArrayLength(blob_java); StorageRecord dest( std::to_string(id_java), - token_utf, + token_utf != nullptr ? token_utf : "", latency, persistence, timestamp, StorageBlob(start, end), retryCount, reservedUntil); - env->ReleaseStringUTFChars(tenantToken_java, token_utf); + if (token_utf != nullptr) + { + env->ReleaseStringUTFChars(tenantToken_java, token_utf); + } env->ReleaseByteArrayElements(blob_java, reinterpret_cast(start), 0); env.popLocalFrame(); @@ -1036,8 +1039,11 @@ namespace MAT_NS_BEGIN { auto utf = env->GetStringUTFChars(java_value, nullptr); ThrowRuntime(env, "copy setting value"); - result = utf; - env->ReleaseStringUTFChars(java_value, utf); + if (utf != nullptr) + { + result = utf; + env->ReleaseStringUTFChars(java_value, utf); + } } return result; } @@ -1277,14 +1283,17 @@ namespace MAT_NS_BEGIN auto blob_end = blob_store + blob_length; records.emplace_back( std::to_string(id_j), - tenant_utf, + tenant_utf != nullptr ? tenant_utf : "", latency, persistence, timestamp, StorageBlob(blob_store, blob_end), retryCount, reservedUntil); - env->ReleaseStringUTFChars(tenant_j, tenant_utf); + if (tenant_utf != nullptr) + { + env->ReleaseStringUTFChars(tenant_j, tenant_utf); + } env->ReleaseByteArrayElements(blob_j, elements, 0); env.popLocalFrame(); } From 772ce61e0abc0c37a21f54c3006eb00be15fe6af Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 25 Jun 2026 09:50:14 -0500 Subject: [PATCH 04/20] Guard tenant_j for null before GetStringUTFChars in GetRecords (Copilot review) Unlike the other JNI string reads in this file, the GetRecords() path called env->GetStringUTFChars(tenant_j, ...) with neither a pre-call null check on the jstring nor a following ThrowRuntime() exception check. Passing a null jstring to GetStringUTFChars is undefined per the JNI spec (and can leave a pending NullPointerException in flight). Only call GetStringUTFChars when tenant_j is non-null; the downstream already tolerates a null tenant_utf (defaults to "" and skips the guarded ReleaseStringUTFChars). Validated: NDK 27 clang++ --target=aarch64-linux-android24 -fsyntax-only compiles OfflineStorage_Room.cpp (USE_ROOM) cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/OfflineStorage_Room.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index ccc7ae232..85f90b38b 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -1355,7 +1355,9 @@ namespace MAT_NS_BEGIN auto id_j = env->GetLongField(record, id_id); auto tenant_j = static_cast(env->GetObjectField(record, tenantToken_id)); - auto tenant_utf = env->GetStringUTFChars(tenant_j, nullptr); + const char* tenant_utf = (tenant_j != nullptr) + ? env->GetStringUTFChars(tenant_j, nullptr) + : nullptr; auto latency = static_cast(env->GetIntField(record, latency_id)); auto persistence = static_cast(env->GetIntField(record, From 80f865c84b7c5867a70357505930bbb72e8ada7f Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 25 Jun 2026 10:13:23 -0500 Subject: [PATCH 05/20] Guard remaining jstring inputs before GetStringUTFChars (Copilot review) Copilot's re-review flagged two more JNI string reads where the result was null-checked but the jstring input was passed to GetStringUTFChars unguarded, which crashes (or leaves a pending exception) before the result check when the jstring is null: - Signals_jni.cpp nativeInitialize: only read base_url when non-null; if the read itself fails (e.g. OOM) return immediately rather than continuing JNI calls with an exception pending, matching the nativeLog(signal_item_json) site. - OfflineStorage_Room.cpp ByTenant releaseRecords (token) and GetRecords-by-tenant (tenantToken_java): only call GetStringUTFChars when the jstring is non-null; the downstream already tolerates a null result (skips/defaults). The getSetting site is already guarded by an enclosing if (java_value). Validated: NDK 27 clang++ --target=aarch64-linux-android24 -fsyntax-only compiles both files cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/Signals_jni.cpp | 9 +++++++-- lib/offline/OfflineStorage_Room.cpp | 7 +++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 918a84774..ec1be2dce 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -62,8 +62,13 @@ Java_com_microsoft_applications_events_Signals_nativeInitialize(JNIEnv *env, jcl SubstrateSignalsConfiguration config; jboolean isCopy = true; - const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy); - if (convertedValue != nullptr) { + if (base_url != nullptr) { + const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy); + if (convertedValue == nullptr) { + // GetStringUTFChars failed (e.g. OOM) and left a pending exception; + // do not continue making JNI calls with an exception in flight. + return false; + } if (strlen(convertedValue) > 0) { config.ServiceRequestConfig.BaseUrl = convertedValue; } diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index 85f90b38b..362182d94 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -495,7 +495,9 @@ namespace MAT_NS_BEGIN auto tenantToken_java = static_cast(env->GetObjectField(record, tenantToken_id)); ThrowRuntime(env, "get tenant"); - auto token_utf = env->GetStringUTFChars(tenantToken_java, nullptr); + auto token_utf = (tenantToken_java != nullptr) + ? env->GetStringUTFChars(tenantToken_java, nullptr) + : nullptr; ThrowRuntime(env, "string tenant"); auto latency = static_cast(std::max(latency_lb, std::min( @@ -772,7 +774,8 @@ namespace MAT_NS_BEGIN ThrowLogic(env, "Exception fetching token"); auto count = env->GetLongField(byTenant, count_id); ThrowLogic(env, "Exception fetching count"); - auto utf = env->GetStringUTFChars(token, nullptr); + auto utf = (token != nullptr) ? env->GetStringUTFChars(token, nullptr) + : nullptr; ThrowRuntime(env, "Exception fetching token string"); // Skip rather than misattribute dropped records to an empty // tenant token when the string read fails. From 25df775681af7a12e40866580fb1ce1a3fdc8bfc Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 25 Jun 2026 10:25:25 -0500 Subject: [PATCH 06/20] GetRecords: clear pending JNI exception after tenant-token read (Copilot review) If GetStringUTFChars(tenant_j) fails (e.g. OOM) it returns null and leaves a pending JNI exception. GetRecords() previously continued issuing Get*Field calls with that exception in flight, which can make them return defaults. Call ThrowRuntime after the read (as GetAndReserveRecords and the rest of this file already do) to describe+clear the exception, notify the observer, and unwind the batch via the surrounding try/catch. Validated: NDK 27 clang++ -fsyntax-only compiles cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/OfflineStorage_Room.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/offline/OfflineStorage_Room.cpp b/lib/offline/OfflineStorage_Room.cpp index 362182d94..5ea0611e0 100644 --- a/lib/offline/OfflineStorage_Room.cpp +++ b/lib/offline/OfflineStorage_Room.cpp @@ -1361,6 +1361,10 @@ namespace MAT_NS_BEGIN const char* tenant_utf = (tenant_j != nullptr) ? env->GetStringUTFChars(tenant_j, nullptr) : nullptr; + // Clear/handle any pending exception from a failed string read + // (e.g. OOM) before making further JNI calls, consistent with the + // other read paths in this file. + ThrowRuntime(env, "string tenant"); auto latency = static_cast(env->GetIntField(record, latency_id)); auto persistence = static_cast(env->GetIntField(record, From 127468060106b6232e75d0d9cd4c4358782e7213 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 25 Jun 2026 10:36:56 -0500 Subject: [PATCH 07/20] JStringToStdString: clear pending JNI exception on failed read (Copilot review) When GetStringUTFChars returns null (e.g. OOM) it leaves a pending Java exception. JStringToStdString returned "" without clearing it, so callers kept making JNI calls with an exception in flight. Clear the pending exception before returning, consistent with the ExceptionClear handling elsewhere in the JNI layer. Keeps the existing string-returning contract rather than redesigning the helper's signature, so no call sites change. Validated: NDK 27 clang++ -fsyntax-only compiles cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/JniConvertors.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/jni/JniConvertors.cpp b/lib/jni/JniConvertors.cpp index 61eceafff..e1b0afe5a 100644 --- a/lib/jni/JniConvertors.cpp +++ b/lib/jni/JniConvertors.cpp @@ -13,8 +13,13 @@ 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) + if (jstr_utf == nullptr) { + // GetStringUTFChars failed (e.g. OOM) and left a pending exception. Clear + // it so callers do not keep issuing JNI calls with an exception in flight. + if (env->ExceptionCheck() == JNI_TRUE) + env->ExceptionClear(); return ""; + } std::string str(jstr_utf, jstr_utf + jstr_length); env->ReleaseStringUTFChars(jstr, jstr_utf); return str; From 76d75ed1cd1cb51ba4d263c1a869f3d447fec036 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 25 Jun 2026 10:47:02 -0500 Subject: [PATCH 08/20] UWP: include directly for std::exception catch (Copilot review) The DeviceFamilyVersion parse added a catch(const std::exception&) but the file only pulled in transitively. Include it explicitly so the build does not depend on transitive include order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp b/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp index 8c4ff7f66..e8b1097eb 100644 --- a/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp +++ b/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp @@ -5,6 +5,7 @@ #include "pal/PAL.hpp" #include +#include #include "ISystemInformation.hpp" #include "pal/SystemInformationImpl.hpp" From e027995b744f4a6284fa468bc40f32bc38195c39 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 12:48:05 -0500 Subject: [PATCH 09/20] Bound the HTTP cancel drains so they cannot spin or hang (issue #1437) Both HttpClientManager::cancelAllRequests() and HttpClient_WinInet::CancelAllRequests() waited for their in-flight request lists to drain with an unbounded poll loop. A previously-merged fix stopped the 100% CPU burn (locked the empty() check + sleep) but left the loop unbounded: if the drainer never runs -- the SDK task dispatcher stalls/stops for the manager, or a WinInet callback stalls -- the loop still blocks forever, and cancelAllRequests holds the LogManager lock the whole time, freezing LogEvent (issue #1437, a macOS spindump). Replace both poll loops with a condition variable signaled from the drain site (HttpClientManager::onHttpResponse / HttpClient_WinInet::erase), bounded by a timeout. The CV makes the common case drain in microseconds; the timeout is a last-resort safety valve so the drain can never spin or block indefinitely (and so the LogManager lock is held for at most the timeout, not forever). Callbacks/requests are deliberately NOT force-cleared on timeout: the HTTP client still owns them and invokes them later, so deleting them here would use-after-free (the destructor is not a drain barrier). The timeout is therefore a bounded best-effort valve, not the primary drain. Adds HttpClientManagerTests.CancelAllRequests_TimesOutInsteadOfHanging, which holds a request open and asserts cancelAllRequests returns within the (shortened) timeout instead of hanging, then completes the outstanding callback to confirm it is still safe after the drain was abandoned. Verified: full FuncTests suite (39 tests) passes on Linux (CV drain, no regression); the new unit test passes; HttpClient_WinInet.cpp compiles under MSVC. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClientManager.cpp | 31 ++++++++++------ lib/http/HttpClientManager.hpp | 10 +++++ lib/http/HttpClient_WinInet.cpp | 28 +++++++------- lib/http/HttpClient_WinInet.hpp | 9 +++++ tests/unittests/HttpClientManagerTests.cpp | 43 ++++++++++++++++++++++ 5 files changed, 95 insertions(+), 26 deletions(-) diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index 0de14e085..e3489a4fb 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -137,6 +137,8 @@ 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; @@ -152,19 +154,24 @@ namespace MAT_NS_BEGIN { { 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 (;;) + // Wait for in-flight callbacks to drain before the caller proceeds (e.g. to + // shutdown, which destroys state these callbacks still reference). Use a + // condition variable signaled from onHttpResponse rather than a poll loop, + // and cap the wait so a stalled dispatcher or HTTP stack can never make this + // spin or block forever -- the failure reported in issue #1437, where this + // burned 100% CPU while holding the LogManager lock. Callbacks are NOT + // force-cleared on timeout: the HTTP client still owns them and will invoke + // them later, so deleting them here would use-after-free. The bounded wait + // is a last-resort safety valve; the common case drains in microseconds via + // the CV. + std::unique_lock lock(m_httpCallbacksMtx); + if (!m_httpCallbacksCV.wait_for(lock, m_cancelDrainTimeout, + [this] { return m_httpCallbacks.empty(); })) { - { - LOCKGUARD(m_httpCallbacksMtx); - if (m_httpCallbacks.empty()) - { - return; - } - } - std::this_thread::sleep_for(std::chrono::milliseconds(50)); + LOG_ERROR("cancelAllRequests: %zu HTTP callback(s) did not drain within %lld ms; " + "proceeding to avoid blocking indefinitely", + m_httpCallbacks.size(), + static_cast(m_cancelDrainTimeout.count())); } } diff --git a/lib/http/HttpClientManager.hpp b/lib/http/HttpClientManager.hpp index e8214d631..f889551a1 100644 --- a/lib/http/HttpClientManager.hpp +++ b/lib/http/HttpClientManager.hpp @@ -12,6 +12,8 @@ #include #include +#include +#include namespace MAT_NS_BEGIN { @@ -62,6 +64,14 @@ class HttpClientManager ITaskDispatcher& m_taskDispatcher; mutable std::recursive_mutex m_httpCallbacksMtx; std::list 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 (issue #1437). Adjustable so tests can + // exercise the timeout path without a long wait. + std::chrono::milliseconds m_cancelDrainTimeout{std::chrono::seconds(30)}; }; } MAT_NS_END diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index 9c920682d..d846e68ea 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/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; } @@ -534,21 +536,19 @@ void HttpClient_WinInet::CancelAllRequests() for (const auto &id : ids) CancelRequestAsync(id); - // wait for all destructors to run. Read m_requests under the lock each - // iteration; erase() runs on the WinInet callback thread under the same lock. - bool done; + // 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, capped by a timeout so a stalled WinInet callback can never make + // this spin or block forever (issue #1437). Requests are not force-removed on + // timeout: WinInet still owns them and will invoke their callbacks later. + std::unique_lock lock(m_requestsMutex); + if (!m_requestsCV.wait_for(lock, m_cancelDrainTimeout, + [this] { return m_requests.empty(); })) { - LOCKGUARD(m_requestsMutex); - done = m_requests.empty(); - } - while (!done) - { - PAL::sleep(100); - std::this_thread::yield(); - { - LOCKGUARD(m_requestsMutex); - done = m_requests.empty(); - } + LOG_ERROR("CancelAllRequests: %zu request(s) did not drain within %lld ms; " + "proceeding to avoid blocking indefinitely", + m_requests.size(), + static_cast(m_cancelDrainTimeout.count())); } } diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index 7e9379ded..4fe2aa131 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/lib/http/HttpClient_WinInet.hpp @@ -12,6 +12,10 @@ #include "ILogManager.hpp" +#include +#include +#include + namespace MAT_NS_BEGIN { #ifndef _WININET_ @@ -43,6 +47,11 @@ class HttpClient_WinInet : public IHttpClient { HINTERNET m_hInternet; std::recursive_mutex m_requestsMutex; std::map m_requests; + // Signaled from erase() when a request is removed, so CancelAllRequests can + // drain via a condition variable instead of a poll loop, capped by a timeout + // so a stalled callback can never make the drain spin/hang forever (issue #1437). + std::condition_variable_any m_requestsCV; + std::chrono::milliseconds m_cancelDrainTimeout{std::chrono::seconds(30)}; static unsigned s_nextRequestId; bool m_msRootCheck; friend class WinInetRequestWrapper; diff --git a/tests/unittests/HttpClientManagerTests.cpp b/tests/unittests/HttpClientManagerTests.cpp index b2e34a99e..3c1b5690f 100644 --- a/tests/unittests/HttpClientManagerTests.cpp +++ b/tests/unittests/HttpClientManagerTests.cpp @@ -23,6 +23,11 @@ class HttpClientManager4Test : public HttpClientManager { { onHttpResponse(callback); } + + void setCancelDrainTimeout(std::chrono::milliseconds t) + { + m_cancelDrainTimeout = t; + } }; class HttpClientManagerTests : public StrictMock { @@ -74,3 +79,41 @@ TEST_F(HttpClientManagerTests, HandlesRequestFlow) EXPECT_THAT(ctx->httpResponse, rspRef); EXPECT_THAT(ctx->durationMs, Gt(199)); } + +// Regression test for issue #1437: cancelAllRequests() must not spin/hang forever +// when an in-flight callback never drains (e.g. the dispatcher or HTTP stack is +// stalled). It waits for the drain via a condition variable, bounded by a timeout. +TEST_F(HttpClientManagerTests, CancelAllRequests_TimesOutInsteadOfHanging) +{ + hcm.setCancelDrainTimeout(std::chrono::milliseconds(150)); + + SimpleHttpRequest* req = new SimpleHttpRequest("stall"); + auto ctx = std::make_shared(); + ctx->httpRequestId = req->GetId(); + ctx->httpRequest = req; + ctx->recordIdsAndTenantIds["r1"] = "t1"; + ctx->latency = EventLatency_Normal; + ctx->packageIds["tenant1-token"] = 0; + + IHttpResponseCallback* callback = nullptr; + EXPECT_CALL(httpClientMock, SendRequestAsync(ctx->httpRequest, _)) + .WillOnce(SaveArg<1>(&callback)); + hcm.sendRequest(ctx); + ASSERT_THAT(callback, NotNull()); + + // The response never arrives, so the callback never drains from m_httpCallbacks. + // cancelAllRequests() must still return, bounded by the drain timeout, rather + // than block forever (MockIHttpClient does not mock CancelAllRequests, so the + // base no-op runs and nothing completes the request). + auto start = std::chrono::steady_clock::now(); + hcm.cancelAllRequests(); + auto elapsedMs = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start).count(); + EXPECT_THAT(elapsedMs, Ge(140)); // waited ~ the timeout + EXPECT_THAT(elapsedMs, Lt(5000)); // but did not hang + + // Drain the still-outstanding callback so nothing leaks, and confirm it is still + // safe to complete after cancelAllRequests abandoned the drain. + EXPECT_CALL(*this, resultRequestDone(ctx)).WillOnce(Return()); + callback->OnHttpResponse(new SimpleHttpResponse("stall")); +} From c92066e8d45ba99066bd942d63e51bebfde66385 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 13:18:10 -0500 Subject: [PATCH 10/20] Distinguish best-effort (pause) from full-drain (teardown) HTTP cancel Address Copilot review on cancelAllRequests. The previous change bounded every cancel drain with a timeout, which -- as Copilot noted -- can return with callbacks still in flight on the shutdown/destructor paths, where the caller then destroys state a late callback references (use-after-free). Split the two intents: - HttpClientManager::cancelAllRequests(bool bestEffort). bestEffort=true (pause, which runs under the LogManager lock) caps the wait so it cannot block indefinitely (issue #1437); the manager is not destroyed, so outstanding callbacks stay valid and drain later. bestEffort=false (default; shutdown/cleanup) drains fully -- the lifetime barrier before the referenced state is destroyed. TelemetrySystem::onPause now passes bestEffort=true; onStop/onCleanup keep the full drain. - HttpClient_WinInet::CancelAllRequests() now always drains fully. Its destructor calls it, so a bounded return would let a late WinInet callback touch a destroyed client. WinInet delivers cancellations on its own threads, so this does not depend on the SDK task dispatcher. Both paths use a condition variable signaled from the drain site instead of a poll loop, so neither spins at 100% CPU -- the reported #1437 failure -- whether bounded or full. Verified: FuncTests (39 tests) pass on Linux, including teardown which now takes the full-drain path; HttpClientManagerTests.CancelAllRequests_TimesOutInsteadOfHanging covers the best-effort timeout; HttpClient_WinInet.cpp compiles under MSVC /permissive- /W4. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClientManager.cpp | 41 +++++++++++++--------- lib/http/HttpClientManager.hpp | 8 ++++- lib/http/HttpClient_WinInet.cpp | 18 ++++------ lib/http/HttpClient_WinInet.hpp | 7 ++-- lib/system/TelemetrySystem.cpp | 5 ++- tests/unittests/HttpClientManagerTests.cpp | 8 ++--- 6 files changed, 48 insertions(+), 39 deletions(-) diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index e3489a4fb..42393a137 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -150,28 +150,35 @@ namespace MAT_NS_BEGIN { return true; } - void HttpClientManager::cancelAllRequests() + void HttpClientManager::cancelAllRequests(bool bestEffort) { cancelAllRequestsAsync(); - // Wait for in-flight callbacks to drain before the caller proceeds (e.g. to - // shutdown, which destroys state these callbacks still reference). Use a - // condition variable signaled from onHttpResponse rather than a poll loop, - // and cap the wait so a stalled dispatcher or HTTP stack can never make this - // spin or block forever -- the failure reported in issue #1437, where this - // burned 100% CPU while holding the LogManager lock. Callbacks are NOT - // force-cleared on timeout: the HTTP client still owns them and will invoke - // them later, so deleting them here would use-after-free. The bounded wait - // is a last-resort safety valve; the common case drains in microseconds via - // the CV. + // 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 (issue #1437). std::unique_lock lock(m_httpCallbacksMtx); - if (!m_httpCallbacksCV.wait_for(lock, m_cancelDrainTimeout, - [this] { return m_httpCallbacks.empty(); })) + if (bestEffort) { - LOG_ERROR("cancelAllRequests: %zu HTTP callback(s) did not drain within %lld ms; " - "proceeding to avoid blocking indefinitely", - m_httpCallbacks.size(), - static_cast(m_cancelDrainTimeout.count())); + // Pause (and similar) must not block indefinitely -- the caller may hold + // the LogManager lock (the #1437 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. + if (!m_httpCallbacksCV.wait_for(lock, m_cancelDrainTimeout, + [this] { return m_httpCallbacks.empty(); })) + { + LOG_WARN("cancelAllRequests: %zu callback(s) still draining after %lld ms (best-effort)", + m_httpCallbacks.size(), static_cast(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(); }); } } diff --git a/lib/http/HttpClientManager.hpp b/lib/http/HttpClientManager.hpp index f889551a1..36d2f79d8 100644 --- a/lib/http/HttpClientManager.hpp +++ b/lib/http/HttpClientManager.hpp @@ -30,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 { diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index d846e68ea..bc3f4665b 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -538,18 +538,14 @@ void HttpClient_WinInet::CancelAllRequests() // 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, capped by a timeout so a stalled WinInet callback can never make - // this spin or block forever (issue #1437). Requests are not force-removed on - // timeout: WinInet still owns them and will invoke their callbacks later. + // poll loop so this never spins at 100% CPU while draining (issue #1437). This is + // a full drain barrier: the destructor calls CancelAllRequests(), and returning + // early with requests still in flight would let a late WinInet callback invoke + // WinInetRequestWrapper::OnHttpResponse -> m_parent.erase() on a destroyed + // client. WinInet delivers the cancellation callbacks on its own threads, so the + // wait completes without depending on the SDK task dispatcher. std::unique_lock lock(m_requestsMutex); - if (!m_requestsCV.wait_for(lock, m_cancelDrainTimeout, - [this] { return m_requests.empty(); })) - { - LOG_ERROR("CancelAllRequests: %zu request(s) did not drain within %lld ms; " - "proceeding to avoid blocking indefinitely", - m_requests.size(), - static_cast(m_cancelDrainTimeout.count())); - } + m_requestsCV.wait(lock, [this] { return m_requests.empty(); }); } /// diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index 4fe2aa131..b1a6f63bc 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/lib/http/HttpClient_WinInet.hpp @@ -13,7 +13,6 @@ #include "ILogManager.hpp" #include -#include #include namespace MAT_NS_BEGIN { @@ -47,11 +46,9 @@ class HttpClient_WinInet : public IHttpClient { HINTERNET m_hInternet; std::recursive_mutex m_requestsMutex; std::map m_requests; - // Signaled from erase() when a request is removed, so CancelAllRequests can - // drain via a condition variable instead of a poll loop, capped by a timeout - // so a stalled callback can never make the drain spin/hang forever (issue #1437). + // 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, issue #1437). std::condition_variable_any m_requestsCV; - std::chrono::milliseconds m_cancelDrainTimeout{std::chrono::seconds(30)}; static unsigned s_nextRequestId; bool m_msRootCheck; friend class WinInetRequestWrapper; diff --git a/lib/system/TelemetrySystem.cpp b/lib/system/TelemetrySystem.cpp index 24ad34ba9..c33278cc5 100644 --- a/lib/system/TelemetrySystem.cpp +++ b/lib/system/TelemetrySystem.cpp @@ -141,7 +141,10 @@ namespace MAT_NS_BEGIN { { bool result = true; result &= tpm.pause(); - hcm.cancelAllRequests(); + // Best-effort: pause runs under the LogManager lock and must not block + // indefinitely if a callback is slow to drain (issue #1437). The system + // is not being torn down, so outstanding callbacks stay valid. + hcm.cancelAllRequests(/* bestEffort */ true); return result; }; diff --git a/tests/unittests/HttpClientManagerTests.cpp b/tests/unittests/HttpClientManagerTests.cpp index 3c1b5690f..6e2e36da8 100644 --- a/tests/unittests/HttpClientManagerTests.cpp +++ b/tests/unittests/HttpClientManagerTests.cpp @@ -102,11 +102,11 @@ TEST_F(HttpClientManagerTests, CancelAllRequests_TimesOutInsteadOfHanging) ASSERT_THAT(callback, NotNull()); // The response never arrives, so the callback never drains from m_httpCallbacks. - // cancelAllRequests() must still return, bounded by the drain timeout, rather - // than block forever (MockIHttpClient does not mock CancelAllRequests, so the - // base no-op runs and nothing completes the request). + // The best-effort (pause) drain must still return, bounded by the drain timeout, + // rather than block forever (MockIHttpClient does not mock CancelAllRequests, so + // the base no-op runs and nothing completes the request). auto start = std::chrono::steady_clock::now(); - hcm.cancelAllRequests(); + hcm.cancelAllRequests(/* bestEffort */ true); auto elapsedMs = std::chrono::duration_cast( std::chrono::steady_clock::now() - start).count(); EXPECT_THAT(elapsedMs, Ge(140)); // waited ~ the timeout From d573254b8aa17bb33d02a884ed70c30526496c54 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 8 Jul 2026 22:46:17 -0500 Subject: [PATCH 11/20] Clear pending JNI exception before returning false in Signals_jni Addresses two Copilot review comments (Signals_jni.cpp:36 and :71). When GetStringUTFChars returns null it typically leaves a pending Java exception (e.g. OutOfMemoryError). Returning false without clearing it means the JVM throws that exception at the Java call site, so the caller sees the exception rather than the intended graceful alse -- contradicting the comment. Call env->ExceptionClear() before returning false at both sites so the caller observes a clean false (telemetry should degrade gracefully rather than propagate an OOM into the host app). ExceptionClear is one of the few JNI functions safe to call with an exception in flight, and no other JNI calls are made before return. Validated: the exact JNI usage (GetStringUTFChars + ExceptionClear + ReleaseStringUTFChars) compiles clean under the NDK aarch64-linux-android24 clang with -Wall -Werror. (A full Gradle build of this TU isn't possible in this worktree -- the private lib/modules submodule providing Signals.hpp is absent.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/Signals_jni.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index ec1be2dce..75bfef0bb 100644 --- a/lib/jni/Signals_jni.cpp +++ b/lib/jni/Signals_jni.cpp @@ -30,8 +30,11 @@ Java_com_microsoft_applications_events_Signals_sendSignal(JNIEnv *env, } const char *signalItemJson = (env)->GetStringUTFChars(signal_item_json, &isCopy); if (signalItemJson == nullptr) { - // GetStringUTFChars returned null (e.g. OOM, with a pending exception); - // there is nothing valid to log. + // GetStringUTFChars returned null (e.g. OOM), which leaves a pending Java + // exception. Clear it so the caller observes a clean `false` return instead + // of the exception being thrown at the Java call site. ExceptionClear is one + // of the few JNI calls that is safe to make with an exception in flight. + env->ExceptionClear(); return false; } @@ -65,8 +68,12 @@ Java_com_microsoft_applications_events_Signals_nativeInitialize(JNIEnv *env, jcl if (base_url != nullptr) { const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy); if (convertedValue == nullptr) { - // GetStringUTFChars failed (e.g. OOM) and left a pending exception; - // do not continue making JNI calls with an exception in flight. + // GetStringUTFChars failed (e.g. OOM), leaving a pending Java exception. + // Clear it so the caller observes a clean `false` instead of the + // exception being thrown at the Java call site. ExceptionClear is safe + // to call with an exception in flight, and we make no other JNI calls + // before returning. + env->ExceptionClear(); return false; } if (strlen(convertedValue) > 0) { From 1e7a93df3bdcc8e41655c1dccfdf96bc887c98a6 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 9 Jul 2026 11:01:32 -0500 Subject: [PATCH 12/20] Drop issue-number references from code comments Reword the HTTP cancel-drain comments to describe the behavior without citing tracking numbers; no code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClientManager.cpp | 4 ++-- lib/http/HttpClientManager.hpp | 2 +- lib/http/HttpClient_WinInet.cpp | 2 +- lib/http/HttpClient_WinInet.hpp | 2 +- lib/system/TelemetrySystem.cpp | 2 +- tests/unittests/HttpClientManagerTests.cpp | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index 42393a137..b9daa2656 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -156,12 +156,12 @@ namespace MAT_NS_BEGIN { // 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 (issue #1437). + // holding the LogManager lock. std::unique_lock lock(m_httpCallbacksMtx); if (bestEffort) { // Pause (and similar) must not block indefinitely -- the caller may hold - // the LogManager lock (the #1437 spindump was PauseTransmission). The + // 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. if (!m_httpCallbacksCV.wait_for(lock, m_cancelDrainTimeout, diff --git a/lib/http/HttpClientManager.hpp b/lib/http/HttpClientManager.hpp index 36d2f79d8..1f4ca3091 100644 --- a/lib/http/HttpClientManager.hpp +++ b/lib/http/HttpClientManager.hpp @@ -75,7 +75,7 @@ class HttpClientManager 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 (issue #1437). Adjustable so tests can + // 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)}; }; diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index bc3f4665b..c2f35f408 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -538,7 +538,7 @@ void HttpClient_WinInet::CancelAllRequests() // 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 (issue #1437). This is + // poll loop so this never spins at 100% CPU while draining. This is // a full drain barrier: the destructor calls CancelAllRequests(), and returning // early with requests still in flight would let a late WinInet callback invoke // WinInetRequestWrapper::OnHttpResponse -> m_parent.erase() on a destroyed diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index b1a6f63bc..8ce905eff 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/lib/http/HttpClient_WinInet.hpp @@ -47,7 +47,7 @@ class HttpClient_WinInet : public IHttpClient { std::recursive_mutex m_requestsMutex; std::map 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, issue #1437). + // 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; diff --git a/lib/system/TelemetrySystem.cpp b/lib/system/TelemetrySystem.cpp index c33278cc5..2e5059b47 100644 --- a/lib/system/TelemetrySystem.cpp +++ b/lib/system/TelemetrySystem.cpp @@ -142,7 +142,7 @@ namespace MAT_NS_BEGIN { bool result = true; result &= tpm.pause(); // Best-effort: pause runs under the LogManager lock and must not block - // indefinitely if a callback is slow to drain (issue #1437). The system + // indefinitely if a callback is slow to drain. The system // is not being torn down, so outstanding callbacks stay valid. hcm.cancelAllRequests(/* bestEffort */ true); return result; diff --git a/tests/unittests/HttpClientManagerTests.cpp b/tests/unittests/HttpClientManagerTests.cpp index 6e2e36da8..26272fcf1 100644 --- a/tests/unittests/HttpClientManagerTests.cpp +++ b/tests/unittests/HttpClientManagerTests.cpp @@ -80,7 +80,7 @@ TEST_F(HttpClientManagerTests, HandlesRequestFlow) EXPECT_THAT(ctx->durationMs, Gt(199)); } -// Regression test for issue #1437: cancelAllRequests() must not spin/hang forever +// Regression test: cancelAllRequests() must not spin/hang forever // when an in-flight callback never drains (e.g. the dispatcher or HTTP stack is // stalled). It waits for the drain via a condition variable, bounded by a timeout. TEST_F(HttpClientManagerTests, CancelAllRequests_TimesOutInsteadOfHanging) From 08722d26ced2fcd62da73e0a69cefdfcd0ac0331 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 9 Jul 2026 11:25:19 -0500 Subject: [PATCH 13/20] Drop issue-number reference from JniConvertors comment Reword the event-type comment to describe the behavior without citing a tracking number; no code change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/jni/JniConvertors.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jni/JniConvertors.cpp b/lib/jni/JniConvertors.cpp index 041114d47..7ad8bcd2b 100644 --- a/lib/jni/JniConvertors.cpp +++ b/lib/jni/JniConvertors.cpp @@ -167,7 +167,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 From 348ea88c1202a19c38c08c449ddf15294614ad36 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 9 Jul 2026 11:27:46 -0500 Subject: [PATCH 14/20] Only add for the new member in HttpClient_WinInet This change adds a std::condition_variable_any member; is the only include it needs. Drop the include: the pre-existing recursive_mutex member already resolves via the transitively-included pal/PAL.hpp, so adding addressed a pre-existing concern outside this change's scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_WinInet.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index 8ce905eff..55a59e7dc 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/lib/http/HttpClient_WinInet.hpp @@ -13,7 +13,6 @@ #include "ILogManager.hpp" #include -#include namespace MAT_NS_BEGIN { From ceddf4a2d3b6dc6dc493efad4b1c0c89302af9aa Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Thu, 9 Jul 2026 12:05:00 -0500 Subject: [PATCH 15/20] Re-add to HttpClient_WinInet for a self-contained header The header uses std::recursive_mutex directly, so it should include rather than rely on it being pulled in transitively via pal/PAL.hpp. This keeps the header self-contained (include-what-you-use) alongside for the condition_variable_any member. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_WinInet.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index 55a59e7dc..8ce905eff 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/lib/http/HttpClient_WinInet.hpp @@ -13,6 +13,7 @@ #include "ILogManager.hpp" #include +#include namespace MAT_NS_BEGIN { From 372a74d56e68c6d6aa8c46b2df441939fead3c90 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 14:24:19 -0500 Subject: [PATCH 16/20] Clarify that best-effort pause is only bounded on async-handler platforms The bestEffort drain in cancelAllRequests caps the wait on m_httpCallbacks, but on Windows (USE_SYNC_HTTPRESPONSE_HANDLER) onHttpResponse drains m_httpCallbacks synchronously inside m_httpClient.CancelAllRequests(), so that wait is usually already satisfied and the real blocking is the transport-level wait there (WinInet condition-variable wait, WinRt poll), which is not bounded. Correct the comment so it no longer implies pause is fully bounded on Windows; fully bounding it requires plumbing the deadline into IHttpClient::CancelAllRequests (follow-up). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClientManager.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index b9daa2656..7589210c3 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -160,10 +160,19 @@ namespace MAT_NS_BEGIN { std::unique_lock lock(m_httpCallbacksMtx); if (bestEffort) { - // Pause (and similar) must not block indefinitely -- the caller may hold - // the LogManager lock (the observed spindump was PauseTransmission). The - // manager is NOT being destroyed here, so outstanding callbacks remain + // 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 platforms. On Windows (USE_SYNC_HTTPRESPONSE_HANDLER) + // onHttpResponse drains m_httpCallbacks synchronously inside the + // m_httpClient.CancelAllRequests() call above, so this wait is usually + // already satisfied and the real blocking is the transport-level wait there + // (WinInet condition-variable wait, WinRt poll), which is not yet bounded. + // Fully bounding pause on Windows requires plumbing the deadline down into + // IHttpClient::CancelAllRequests. if (!m_httpCallbacksCV.wait_for(lock, m_cancelDrainTimeout, [this] { return m_httpCallbacks.empty(); })) { From 4ad8c168a87a2e8888fb907aaa14ed3792d29a4e Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 14:50:24 -0500 Subject: [PATCH 17/20] Bound best-effort pause on Windows by plumbing a deadline into CancelAllRequests PauseTransmission could block indefinitely on Windows even with the manager-level drain cap: with USE_SYNC_HTTPRESPONSE_HANDLER the callbacks drain synchronously inside IHttpClient::CancelAllRequests, so the real blocking region is the transport wait (WinInet condition-variable wait, WinRt poll), which was unbounded. Add a bestEffortTimeout parameter to IHttpClient::CancelAllRequests (default zero = full drain for shutdown/destructor; positive = best-effort cap). WinInet and WinRt now bound their drain wait by it; HttpClientManager passes m_cancelDrainTimeout on pause and zero on teardown. Fire-and-forget impls (Apple, CAPI, Android) accept and ignore the parameter. Files: lib/include/public/IHttpClient.hpp, lib/http/HttpClient_WinInet.{hpp,cpp}, HttpClient_WinRt.{hpp,cpp}, HttpClient_Apple.{hpp,mm}, HttpClient_CAPI.{hpp,cpp}, HttpClient_Android.{hpp,cpp}, HttpClientManager.{hpp,cpp}, tests/unittests/HttpClientCAPITests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClientManager.cpp | 17 +++++++++------ lib/http/HttpClientManager.hpp | 2 +- lib/http/HttpClient_Android.cpp | 2 +- lib/http/HttpClient_Android.hpp | 2 +- lib/http/HttpClient_Apple.hpp | 2 +- lib/http/HttpClient_Apple.mm | 2 +- lib/http/HttpClient_CAPI.cpp | 2 +- lib/http/HttpClient_CAPI.hpp | 2 +- lib/http/HttpClient_WinInet.cpp | 29 +++++++++++++++++-------- lib/http/HttpClient_WinInet.hpp | 2 +- lib/http/HttpClient_WinRt.cpp | 8 ++++++- lib/http/HttpClient_WinRt.hpp | 2 +- lib/include/public/IHttpClient.hpp | 10 ++++++++- tests/unittests/HttpClientCAPITests.cpp | 2 +- 14 files changed, 56 insertions(+), 28 deletions(-) diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index 7589210c3..39fb42e8c 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -144,15 +144,19 @@ namespace MAT_NS_BEGIN { delete callback; } - bool HttpClientManager::cancelAllRequestsAsync() + bool HttpClientManager::cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout) { - m_httpClient.CancelAllRequests(); + m_httpClient.CancelAllRequests(bestEffortTimeout); return true; } void HttpClientManager::cancelAllRequests(bool bestEffort) { - cancelAllRequestsAsync(); + // On the synchronous-response-handler platforms (Windows), the transport's + // CancelAllRequests() is where cancellation actually blocks, so pass the + // best-effort deadline down to bound it; the manager-level drain below is the + // bound for the async-handler platforms. A zero timeout means "drain fully". + 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 @@ -169,10 +173,9 @@ namespace MAT_NS_BEGIN { // the async-handler platforms. On Windows (USE_SYNC_HTTPRESPONSE_HANDLER) // onHttpResponse drains m_httpCallbacks synchronously inside the // m_httpClient.CancelAllRequests() call above, so this wait is usually - // already satisfied and the real blocking is the transport-level wait there - // (WinInet condition-variable wait, WinRt poll), which is not yet bounded. - // Fully bounding pause on Windows requires plumbing the deadline down into - // IHttpClient::CancelAllRequests. + // 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 CancelAllRequests above. if (!m_httpCallbacksCV.wait_for(lock, m_cancelDrainTimeout, [this] { return m_httpCallbacks.empty(); })) { diff --git a/lib/http/HttpClientManager.hpp b/lib/http/HttpClientManager.hpp index 1f4ca3091..5f18d8932 100644 --- a/lib/http/HttpClientManager.hpp +++ b/lib/http/HttpClientManager.hpp @@ -63,7 +63,7 @@ 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()); ILogManager& m_logManager; IHttpClient& m_httpClient; diff --git a/lib/http/HttpClient_Android.cpp b/lib/http/HttpClient_Android.cpp index c41a09d19..1c963e46b 100644 --- a/lib/http/HttpClient_Android.cpp +++ b/lib/http/HttpClient_Android.cpp @@ -330,7 +330,7 @@ namespace MAT_NS_BEGIN } } - void HttpClient_Android::CancelAllRequests() + void HttpClient_Android::CancelAllRequests(std::chrono::milliseconds /*bestEffortTimeout*/) { JNIEnv* env; if (s_java_vm->AttachCurrentThread(&env, nullptr) != JNI_OK) diff --git a/lib/http/HttpClient_Android.hpp b/lib/http/HttpClient_Android.hpp index ad7905eca..8fbcfb904 100644 --- a/lib/http/HttpClient_Android.hpp +++ b/lib/http/HttpClient_Android.hpp @@ -178,7 +178,7 @@ namespace MAT_NS_BEGIN IHttpRequest* CreateRequest() override; void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) override; void CancelRequestAsync(std::string const& id) override; - void CancelAllRequests() override; + void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) override; void SetClient(JNIEnv* env, jobject c); void EraseRequest(HttpRequest*); HttpRequest* GetAndRemoveRequest(std::string id); diff --git a/lib/http/HttpClient_Apple.hpp b/lib/http/HttpClient_Apple.hpp index e9b22b1f0..1d6c7746c 100644 --- a/lib/http/HttpClient_Apple.hpp +++ b/lib/http/HttpClient_Apple.hpp @@ -21,7 +21,7 @@ namespace MAT_NS_BEGIN { 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; void Erase(IHttpRequest* req); void Add(IHttpRequest* req); diff --git a/lib/http/HttpClient_Apple.mm b/lib/http/HttpClient_Apple.mm index 449b4c9af..585bcbf32 100644 --- a/lib/http/HttpClient_Apple.mm +++ b/lib/http/HttpClient_Apple.mm @@ -184,7 +184,7 @@ void Cancel() } } -void HttpClient_Apple::CancelAllRequests() +void HttpClient_Apple::CancelAllRequests(std::chrono::milliseconds /*bestEffortTimeout*/) { std::vector ids; { diff --git a/lib/http/HttpClient_CAPI.cpp b/lib/http/HttpClient_CAPI.cpp index 5f344b366..24e804c9e 100644 --- a/lib/http/HttpClient_CAPI.cpp +++ b/lib/http/HttpClient_CAPI.cpp @@ -203,7 +203,7 @@ namespace MAT_NS_BEGIN { } } - void HttpClient_CAPI::CancelAllRequests() + void HttpClient_CAPI::CancelAllRequests(std::chrono::milliseconds /*bestEffortTimeout*/) { LOG_TRACE("Cancelling all CAPI HTTP requests"); std::vector> operations; diff --git a/lib/http/HttpClient_CAPI.hpp b/lib/http/HttpClient_CAPI.hpp index 5fb3cc088..398ebdc9f 100644 --- a/lib/http/HttpClient_CAPI.hpp +++ b/lib/http/HttpClient_CAPI.hpp @@ -20,7 +20,7 @@ namespace MAT_NS_BEGIN { 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; private: http_send_fn_t m_sendFn; diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index c2f35f408..ad465eaad 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -474,7 +474,7 @@ HttpClient_WinInet::HttpClient_WinInet() : HttpClient_WinInet::~HttpClient_WinInet() { - CancelAllRequests(); + CancelAllRequests(std::chrono::milliseconds::zero()); ::InternetCloseHandle(m_hInternet); } @@ -522,7 +522,7 @@ void HttpClient_WinInet::CancelRequestAsync(std::string const& id) } -void HttpClient_WinInet::CancelAllRequests() +void HttpClient_WinInet::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { // vector of all request IDs std::vector ids; @@ -538,14 +538,25 @@ void HttpClient_WinInet::CancelAllRequests() // 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. This is - // a full drain barrier: the destructor calls CancelAllRequests(), and returning - // early with requests still in flight would let a late WinInet callback invoke - // WinInetRequestWrapper::OnHttpResponse -> m_parent.erase() on a destroyed - // client. WinInet delivers the cancellation callbacks on its own threads, so the - // wait completes without depending on the SDK task dispatcher. + // 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 lock(m_requestsMutex); - m_requestsCV.wait(lock, [this] { return m_requests.empty(); }); + 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 + { + // 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(); }); + } } /// diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index 8ce905eff..9158150f9 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/lib/http/HttpClient_WinInet.hpp @@ -31,7 +31,7 @@ class HttpClient_WinInet : public IHttpClient { virtual IHttpRequest* CreateRequest() final; 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; diff --git a/lib/http/HttpClient_WinRt.cpp b/lib/http/HttpClient_WinRt.cpp index db14aa1da..c9398fdb4 100644 --- a/lib/http/HttpClient_WinRt.cpp +++ b/lib/http/HttpClient_WinRt.cpp @@ -337,7 +337,7 @@ namespace MAT_NS_BEGIN { } } - void HttpClient_WinRt::CancelAllRequests() + void HttpClient_WinRt::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { // vector of all request IDs std::vector ids; @@ -353,6 +353,10 @@ namespace MAT_NS_BEGIN { // 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; { std::lock_guard lock(m_requestsMutex); @@ -360,6 +364,8 @@ namespace MAT_NS_BEGIN { } while (!done) { + if (bounded && std::chrono::steady_clock::now() >= deadline) + break; PAL::sleep(100); std::this_thread::yield(); { diff --git a/lib/http/HttpClient_WinRt.hpp b/lib/http/HttpClient_WinRt.hpp index e6352a45b..3d7dee999 100644 --- a/lib/http/HttpClient_WinRt.hpp +++ b/lib/http/HttpClient_WinRt.hpp @@ -35,7 +35,7 @@ class HttpClient_WinRt : public IHttpClient { 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: diff --git a/lib/include/public/IHttpClient.hpp b/lib/include/public/IHttpClient.hpp index 89e5e6cf0..a51f23ed3 100644 --- a/lib/include/public/IHttpClient.hpp +++ b/lib/include/public/IHttpClient.hpp @@ -14,6 +14,7 @@ #include #include #include +#include ///@cond INTERNAL_DOCS namespace MAT_NS_BEGIN @@ -543,7 +544,14 @@ namespace MAT_NS_BEGIN /// A string that contains the ID of the request to cancel. virtual void CancelRequestAsync(std::string const& id) = 0; - virtual void CancelAllRequests() {} + /// + /// Cancels all pending requests. bestEffortTimeout bounds how long the client + /// may block waiting for in-flight requests to drain before returning: the + /// default of zero drains fully (the caller requires all in-flight requests to + /// finish, e.g. at shutdown), while a positive value is a best-effort cap for + /// callers that must not block indefinitely (e.g. pause). + /// + virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout = std::chrono::milliseconds::zero()) { (void)bestEffortTimeout; } /// /// Apply HTTP settings from the log configuration. diff --git a/tests/unittests/HttpClientCAPITests.cpp b/tests/unittests/HttpClientCAPITests.cpp index 0f0e56a7e..8bb909ef4 100644 --- a/tests/unittests/HttpClientCAPITests.cpp +++ b/tests/unittests/HttpClientCAPITests.cpp @@ -192,7 +192,7 @@ TEST(HttpClientCAPITests, CancelAllThenSend) testHelper->SetShouldSend(true); // Cancel all requests (none pending) - httpClient.CancelAllRequests(); + httpClient.CancelAllRequests(std::chrono::milliseconds::zero()); // Build request auto request = httpClient.CreateRequest(); From 55f7c41257ea05dafcf8f1e44b828194daa59adc Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 15:46:07 -0500 Subject: [PATCH 18/20] Address review: preserve CancelAllRequests() override compat and tighten the pause bound - Keep the legacy no-arg CancelAllRequests() virtual and add the timed overload as a separate method whose default forwards to it, so existing IHttpClient consumers that override CancelAllRequests() keep working (source-compatible) while built-in clients override the timed variant. - Treat m_cancelDrainTimeout as the total pause budget: subtract time already spent in the transport-level cancel before waiting on the manager callback drain, so the pause path holds the LogManager lock for at most ~one timeout, not up to 2x. - WinRt bounded poll now sleeps at most the remaining budget (min(100ms, remaining)) instead of a full 100ms, so it does not overshoot bestEffortTimeout by a poll interval. - Relax the manager timeout test's lower bound (Ge(100) for a 150ms timeout) so it still catches immediate-return regressions without being flaky under CI timer jitter. Files: lib/include/public/IHttpClient.hpp, lib/http/HttpClientManager.cpp, HttpClient_WinRt.cpp, tests/unittests/HttpClientManagerTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClientManager.cpp | 11 ++++++++++- lib/http/HttpClient_WinRt.cpp | 19 ++++++++++++++++--- lib/include/public/IHttpClient.hpp | 21 +++++++++++++++------ tests/unittests/HttpClientManagerTests.cpp | 2 +- 4 files changed, 42 insertions(+), 11 deletions(-) diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index 39fb42e8c..a7c3c5744 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -156,6 +156,7 @@ namespace MAT_NS_BEGIN { // CancelAllRequests() is where cancellation actually blocks, so pass the // best-effort deadline down to bound it; the manager-level drain below is the // bound for the async-handler platforms. 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 @@ -176,7 +177,15 @@ namespace MAT_NS_BEGIN { // 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 CancelAllRequests above. - if (!m_httpCallbacksCV.wait_for(lock, m_cancelDrainTimeout, + // + // 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::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)", diff --git a/lib/http/HttpClient_WinRt.cpp b/lib/http/HttpClient_WinRt.cpp index c9398fdb4..9c00b60e5 100644 --- a/lib/http/HttpClient_WinRt.cpp +++ b/lib/http/HttpClient_WinRt.cpp @@ -364,9 +364,22 @@ namespace MAT_NS_BEGIN { } while (!done) { - if (bounded && std::chrono::steady_clock::now() >= deadline) - break; - PAL::sleep(100); + 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(deadline - now).count(); + if (remainingMs < 1) remainingMs = 1; + if (remainingMs > 100) remainingMs = 100; + PAL::sleep(static_cast(remainingMs)); + } + else + { + PAL::sleep(100); + } std::this_thread::yield(); { std::lock_guard lock(m_requestsMutex); diff --git a/lib/include/public/IHttpClient.hpp b/lib/include/public/IHttpClient.hpp index a51f23ed3..8b2283c56 100644 --- a/lib/include/public/IHttpClient.hpp +++ b/lib/include/public/IHttpClient.hpp @@ -545,13 +545,22 @@ namespace MAT_NS_BEGIN virtual void CancelRequestAsync(std::string const& id) = 0; /// - /// Cancels all pending requests. bestEffortTimeout bounds how long the client - /// may block waiting for in-flight requests to drain before returning: the - /// default of zero drains fully (the caller requires all in-flight requests to - /// finish, e.g. at shutdown), while a positive value is a best-effort cap for - /// callers that must not block indefinitely (e.g. pause). + /// Cancels all pending requests, draining fully before returning. Overriding + /// this method remains supported for backward compatibility: the SDK invokes the + /// timed overload below, whose default implementation forwards here, so existing + /// IHttpClient implementations that only override CancelAllRequests() keep working. /// - virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout = std::chrono::milliseconds::zero()) { (void)bestEffortTimeout; } + virtual void CancelAllRequests() {} + + /// + /// Cancels all pending requests, bounding how long the client may block waiting + /// for in-flight requests to drain. A zero timeout drains fully (the caller + /// requires all in-flight requests to finish, e.g. at shutdown); a positive value + /// is a best-effort cap for callers that must not block indefinitely (e.g. pause). + /// The default implementation ignores the timeout and forwards to + /// CancelAllRequests(). + /// + virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { (void)bestEffortTimeout; CancelAllRequests(); } /// /// Apply HTTP settings from the log configuration. diff --git a/tests/unittests/HttpClientManagerTests.cpp b/tests/unittests/HttpClientManagerTests.cpp index 26272fcf1..2271154c8 100644 --- a/tests/unittests/HttpClientManagerTests.cpp +++ b/tests/unittests/HttpClientManagerTests.cpp @@ -109,7 +109,7 @@ TEST_F(HttpClientManagerTests, CancelAllRequests_TimesOutInsteadOfHanging) hcm.cancelAllRequests(/* bestEffort */ true); auto elapsedMs = std::chrono::duration_cast( std::chrono::steady_clock::now() - start).count(); - EXPECT_THAT(elapsedMs, Ge(140)); // waited ~ the timeout + EXPECT_THAT(elapsedMs, Ge(100)); // waited a meaningful fraction of the 150ms timeout, not an immediate return EXPECT_THAT(elapsedMs, Lt(5000)); // but did not hang // Drain the still-outstanding callback so nothing leaks, and confirm it is still From 53b1e72967b8da8ed21bb8932374fddddf69b57b Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 16:02:45 -0500 Subject: [PATCH 19/20] Clarify CancelAllRequests compatibility: source-compatible, not ABI-stable The overload preserves source compatibility for existing IHttpClient overrides, but adding a virtual changes the vtable, so binary implementations must be recompiled -- consistent with the SDK's C++ classes not being ABI-stable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/include/public/IHttpClient.hpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/include/public/IHttpClient.hpp b/lib/include/public/IHttpClient.hpp index 8b2283c56..1e1816fbd 100644 --- a/lib/include/public/IHttpClient.hpp +++ b/lib/include/public/IHttpClient.hpp @@ -545,10 +545,13 @@ namespace MAT_NS_BEGIN virtual void CancelRequestAsync(std::string const& id) = 0; /// - /// Cancels all pending requests, draining fully before returning. Overriding - /// this method remains supported for backward compatibility: the SDK invokes the - /// timed overload below, whose default implementation forwards here, so existing - /// IHttpClient implementations that only override CancelAllRequests() keep working. + /// Cancels all pending requests, draining fully before returning. Overriding this + /// method remains source-compatible: the SDK invokes the timed overload below, + /// whose default implementation forwards here, so existing IHttpClient + /// implementations that only override CancelAllRequests() keep compiling and are + /// still invoked. Adding the overload changes the vtable, so IHttpClient + /// implementations shipped as binaries must be recompiled -- consistent with the + /// SDK's C++ classes not being ABI-stable (only the flat C API in mat.h is). /// virtual void CancelAllRequests() {} From 15c9eb1a7ea94e1074ac3c2ffb15a7025fd363f9 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Fri, 10 Jul 2026 16:16:24 -0500 Subject: [PATCH 20/20] Keep the no-arg CancelAllRequests() working on every built-in client With the legacy no-arg virtual restored on IHttpClient, an impl that overrode only the timed overload would let a CancelAllRequests() call through an IHttpClient reference hit the base no-op instead of cancelling. Each built-in client (WinInet, WinRt, Apple, CAPI, Android) now also overrides CancelAllRequests() to forward to the timed overload with a zero (full-drain) timeout, so both signatures cancel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/http/HttpClient_Android.hpp | 1 + lib/http/HttpClient_Apple.hpp | 1 + lib/http/HttpClient_CAPI.hpp | 1 + lib/http/HttpClient_WinInet.hpp | 1 + lib/http/HttpClient_WinRt.hpp | 1 + 5 files changed, 5 insertions(+) diff --git a/lib/http/HttpClient_Android.hpp b/lib/http/HttpClient_Android.hpp index 8fbcfb904..799ade261 100644 --- a/lib/http/HttpClient_Android.hpp +++ b/lib/http/HttpClient_Android.hpp @@ -179,6 +179,7 @@ namespace MAT_NS_BEGIN void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) override; void CancelRequestAsync(std::string const& id) override; void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) override; + void CancelAllRequests() override { CancelAllRequests(std::chrono::milliseconds::zero()); } void SetClient(JNIEnv* env, jobject c); void EraseRequest(HttpRequest*); HttpRequest* GetAndRemoveRequest(std::string id); diff --git a/lib/http/HttpClient_Apple.hpp b/lib/http/HttpClient_Apple.hpp index 1d6c7746c..242aeaa6c 100644 --- a/lib/http/HttpClient_Apple.hpp +++ b/lib/http/HttpClient_Apple.hpp @@ -22,6 +22,7 @@ namespace MAT_NS_BEGIN { virtual void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) override; virtual void CancelRequestAsync(std::string const& id) override; virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) override; + void CancelAllRequests() override { CancelAllRequests(std::chrono::milliseconds::zero()); } void Erase(IHttpRequest* req); void Add(IHttpRequest* req); diff --git a/lib/http/HttpClient_CAPI.hpp b/lib/http/HttpClient_CAPI.hpp index 398ebdc9f..31189ab52 100644 --- a/lib/http/HttpClient_CAPI.hpp +++ b/lib/http/HttpClient_CAPI.hpp @@ -21,6 +21,7 @@ namespace MAT_NS_BEGIN { virtual void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) override; virtual void CancelRequestAsync(std::string const& id) override; virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) override; + void CancelAllRequests() override { CancelAllRequests(std::chrono::milliseconds::zero()); } private: http_send_fn_t m_sendFn; diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index 9158150f9..b57486620 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/lib/http/HttpClient_WinInet.hpp @@ -32,6 +32,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(std::chrono::milliseconds bestEffortTimeout) final; + void CancelAllRequests() final { CancelAllRequests(std::chrono::milliseconds::zero()); } virtual void ApplySettings(ILogConfiguration& config) override; diff --git a/lib/http/HttpClient_WinRt.hpp b/lib/http/HttpClient_WinRt.hpp index 3d7dee999..351dbb0d6 100644 --- a/lib/http/HttpClient_WinRt.hpp +++ b/lib/http/HttpClient_WinRt.hpp @@ -36,6 +36,7 @@ class HttpClient_WinRt : public IHttpClient { virtual void SendRequestAsync(IHttpRequest* request, IHttpResponseCallback* callback) override; virtual void CancelRequestAsync(std::string const& id) override; virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) override; + void CancelAllRequests() override { CancelAllRequests(std::chrono::milliseconds::zero()); } HttpClient^ getHttpClient() { return m_httpClient; } protected: