diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index 0de14e085..fe163d1ca 100644 --- a/lib/http/HttpClientManager.cpp +++ b/lib/http/HttpClientManager.cpp @@ -4,6 +4,7 @@ // #include "HttpClientManager.hpp" +#include "IBoundedHttpClientCancel.hpp" #include "utils/StringUtils.hpp" #include "pal/TaskDispatcher.hpp" @@ -11,6 +12,7 @@ #include #include #include +#include #ifdef linux #include @@ -137,34 +139,114 @@ namespace MAT_NS_BEGIN { LOG_TRACE("HTTP remove callback=%p", callback); m_httpCallbacks.remove(callback); + // Wake cancelAllRequests() waiting for the list to drain. + m_httpCallbacksCV.notify_all(); } delete callback; } - bool HttpClientManager::cancelAllRequestsAsync() + bool HttpClientManager::cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout) { + if (bestEffortTimeout > std::chrono::milliseconds::zero()) + { + auto boundedCancel = dynamic_cast(&m_httpClient); + if (boundedCancel != nullptr) + { + boundedCancel->CancelAllRequests(bestEffortTimeout); + return true; + } + + cancelTrackedRequestsAsync(); + return false; + } + m_httpClient.CancelAllRequests(); return true; } - void HttpClientManager::cancelAllRequests() + void HttpClientManager::cancelTrackedRequestsAsync() { - cancelAllRequestsAsync(); - - // Wait for callbacks to drain before shutdown can destroy state that - // those callbacks still use. Keep the list check synchronized and sleep - // between polls so a slow adapter does not burn CPU while draining. - for (;;) + std::vector requestIds; { + LOCKGUARD(m_httpCallbacksMtx); + for (const auto& callback : m_httpCallbacks) { - LOCKGUARD(m_httpCallbacksMtx); - if (m_httpCallbacks.empty()) + if (callback == nullptr || callback->m_ctx == nullptr) + { + continue; + } + + std::string id = callback->m_ctx->httpRequestId; + if (id.empty() && callback->m_ctx->httpRequest != nullptr) + { + id = callback->m_ctx->httpRequest->GetId(); + } + if (!id.empty()) { - return; + requestIds.push_back(id); } } - std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + + for (const auto& id : requestIds) + { + m_httpClient.CancelRequestAsync(id); + } + } + + void HttpClientManager::cancelAllRequests(bool bestEffort) + { + // On clients that opt into bounded cancellation (notably synchronous-response + // Windows transports), the transport's CancelAllRequests() is where + // cancellation actually blocks, so pass the best-effort deadline down to + // bound it. Older clients fall back to per-request async cancel; the + // manager-level drain below is the bound for those async-handler paths. A + // zero timeout means "drain fully". + const auto cancelStart = std::chrono::steady_clock::now(); + cancelAllRequestsAsync(bestEffort ? m_cancelDrainTimeout : std::chrono::milliseconds::zero()); + + // Drain in-flight callbacks via a condition variable signaled from + // onHttpResponse -- never a busy/poll loop, which burned 100% CPU while + // holding the LogManager lock. + std::unique_lock lock(m_httpCallbacksMtx); + if (bestEffort) + { + // Pause (and similar) must not block the caller indefinitely -- it may + // hold the LogManager lock (the observed spindump was PauseTransmission). + // The manager is NOT being destroyed here, so outstanding callbacks remain + // valid and drain later; cap the wait as a safety valve. + // + // This bounds the drain of m_httpCallbacks, which is the blocking region on + // the async-handler and old-style-client fallback paths. On Windows + // (USE_SYNC_HTTPRESPONSE_HANDLER), bounded-capability clients drain + // m_httpCallbacks synchronously inside the transport cancel call above, so + // this wait is usually already satisfied; the real blocking there is the + // transport-level wait (WinInet condition-variable wait, WinRt poll), which + // is bounded by the same best-effort deadline passed into the capability. + // + // Treat m_cancelDrainTimeout as the total budget for the pause: subtract the + // time already spent in the transport cancel so the whole path holds the + // LogManager lock for at most ~m_cancelDrainTimeout, not up to 2x it. + const auto elapsed = std::chrono::duration_cast( + std::chrono::steady_clock::now() - cancelStart); + const auto remaining = (elapsed < m_cancelDrainTimeout) + ? (m_cancelDrainTimeout - elapsed) : std::chrono::milliseconds::zero(); + if (!m_httpCallbacksCV.wait_for(lock, remaining, + [this] { return m_httpCallbacks.empty(); })) + { + LOG_WARN("cancelAllRequests: %zu callback(s) still draining after %lld ms (best-effort)", + m_httpCallbacks.size(), static_cast(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 e8214d631..083cca6dc 100644 --- a/lib/http/HttpClientManager.hpp +++ b/lib/http/HttpClientManager.hpp @@ -12,6 +12,8 @@ #include #include +#include +#include namespace MAT_NS_BEGIN { @@ -28,7 +30,13 @@ class HttpClientManager virtual ~HttpClientManager() noexcept; - void cancelAllRequests(); + // Cancel in-flight requests and drain their callbacks. bestEffort=false (the + // default, used by shutdown/cleanup) drains fully -- it is the lifetime + // barrier before state the callbacks reference is destroyed. bestEffort=true + // (used by pause) caps the wait so it cannot block a caller that holds the + // LogManager lock; the manager is not being destroyed, so outstanding + // callbacks stay valid and drain later. + void cancelAllRequests(bool bestEffort = false); size_t requestCount() const { @@ -55,14 +63,22 @@ class HttpClientManager void handleSendRequest(EventsUploadContextPtr const& ctx); virtual void scheduleOnHttpResponse(HttpCallback* callback); void onHttpResponse(HttpCallback* callback); - bool cancelAllRequestsAsync(); + bool cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout = std::chrono::milliseconds::zero()); + void cancelTrackedRequestsAsync(); ILogManager& m_logManager; IHttpClient& m_httpClient; ITaskDispatcher& m_taskDispatcher; mutable std::recursive_mutex m_httpCallbacksMtx; std::list m_httpCallbacks; + // Signaled from onHttpResponse when a callback is removed, so cancelAllRequests + // can drain via a condition variable instead of a poll loop. + std::condition_variable_any m_httpCallbacksCV; + // Upper bound on how long cancelAllRequests waits for callbacks to drain. A + // last-resort safety valve so a stalled dispatcher/HTTP stack can never make + // the drain spin or block forever. Adjustable so tests can + // exercise the timeout path without a long wait. + std::chrono::milliseconds m_cancelDrainTimeout{std::chrono::seconds(30)}; }; } MAT_NS_END - diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index eaefb2318..789cd6346 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; } @@ -521,6 +523,11 @@ void HttpClient_WinInet::CancelRequestAsync(std::string const& id) void HttpClient_WinInet::CancelAllRequests() +{ + CancelAllRequests(std::chrono::milliseconds::zero()); +} + +void HttpClient_WinInet::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { // vector of all request IDs std::vector ids; @@ -534,11 +541,26 @@ void HttpClient_WinInet::CancelAllRequests() for (const auto &id : ids) CancelRequestAsync(id); - // wait for all destructors to run - while (!m_requests.empty()) + // Wait for all request destructors to run (erase() removes them on the WinInet + // callback thread). Use a condition variable signaled from erase() rather than a + // poll loop so this never spins at 100% CPU while draining. WinInet delivers the + // cancellation callbacks on its own threads, so the wait completes without + // depending on the SDK task dispatcher. + std::unique_lock 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(); }); } } diff --git a/lib/http/HttpClient_WinInet.hpp b/lib/http/HttpClient_WinInet.hpp index 7e9379ded..42b256157 100644 --- a/lib/http/HttpClient_WinInet.hpp +++ b/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 +#include + 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 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 - diff --git a/lib/http/HttpClient_WinRt.cpp b/lib/http/HttpClient_WinRt.cpp index e46e4c49c..a398dfef9 100644 --- a/lib/http/HttpClient_WinRt.cpp +++ b/lib/http/HttpClient_WinRt.cpp @@ -338,6 +338,11 @@ namespace MAT_NS_BEGIN { } void HttpClient_WinRt::CancelAllRequests() + { + CancelAllRequests(std::chrono::milliseconds::zero()); + } + + void HttpClient_WinRt::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { // vector of all request IDs std::vector ids; @@ -351,11 +356,40 @@ namespace MAT_NS_BEGIN { for (const auto &id : ids) CancelRequestAsync(id); - // wait for all destructors to run - while (!m_requests.empty()) + // wait for all destructors to run. Read m_requests under the lock each + // iteration; erase() runs on the PPL continuation thread under the same lock. + // A zero timeout drains fully (shutdown); a positive timeout is a best-effort + // cap so callers such as pause do not block indefinitely. + const bool bounded = bestEffortTimeout > std::chrono::milliseconds::zero(); + const auto deadline = std::chrono::steady_clock::now() + bestEffortTimeout; + bool done; { - PAL::sleep(100); + std::lock_guard 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(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); + done = m_requests.empty(); + } } }; diff --git a/lib/http/HttpClient_WinRt.hpp b/lib/http/HttpClient_WinRt.hpp index e6352a45b..0e10857f1 100644 --- a/lib/http/HttpClient_WinRt.hpp +++ b/lib/http/HttpClient_WinRt.hpp @@ -13,6 +13,7 @@ #include #include "IHttpClient.hpp" +#include "IBoundedHttpClientCancel.hpp" #include "pal/PAL.hpp" #include @@ -28,7 +29,7 @@ namespace MAT_NS_BEGIN { class WinRtRequestWrapper; -class HttpClient_WinRt : public IHttpClient { +class HttpClient_WinRt : public IHttpClient, public IBoundedHttpClientCancel { public: HttpClient_WinRt(); virtual ~HttpClient_WinRt(); @@ -36,6 +37,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() 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 - diff --git a/lib/http/IBoundedHttpClientCancel.hpp b/lib/http/IBoundedHttpClientCancel.hpp new file mode 100644 index 000000000..f832e4678 --- /dev/null +++ b/lib/http/IBoundedHttpClientCancel.hpp @@ -0,0 +1,24 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +#pragma once + +#include "ctmacros.hpp" + +#include + +namespace MAT_NS_BEGIN { + +class IBoundedHttpClientCancel +{ +public: + virtual ~IBoundedHttpClientCancel() noexcept = default; + + // Positive timeout is a best-effort cap. Zero means the caller requires a + // full drain, matching IHttpClient::CancelAllRequests(). + virtual void CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) = 0; +}; + +} MAT_NS_END diff --git a/lib/include/public/IHttpClient.hpp b/lib/include/public/IHttpClient.hpp index 89e5e6cf0..0a9bc4730 100644 --- a/lib/include/public/IHttpClient.hpp +++ b/lib/include/public/IHttpClient.hpp @@ -543,6 +543,10 @@ namespace MAT_NS_BEGIN /// A string that contains the ID of the request to cancel. virtual void CancelRequestAsync(std::string const& id) = 0; + /// + /// Cancels all pending requests, draining fully before returning when the + /// implementation owns a synchronous drain. + /// virtual void CancelAllRequests() {} /// @@ -559,4 +563,3 @@ namespace MAT_NS_BEGIN } MAT_NS_END #endif - diff --git a/lib/jni/JniConvertors.cpp b/lib/jni/JniConvertors.cpp index d944ddff8..7ad8bcd2b 100644 --- a/lib/jni/JniConvertors.cpp +++ b/lib/jni/JniConvertors.cpp @@ -13,6 +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) { + // 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; @@ -160,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 diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 59ca6f32d..75bfef0bb 100644 --- a/lib/jni/Signals_jni.cpp +++ b/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; + 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) { + // 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; + } auto logger = reinterpret_cast(nativeLoggerPtr); EventProperties eventProperties = Signals::CreateEventProperties(signalItemJson); + env->ReleaseStringUTFChars(signal_item_json, signalItemJson); logger->LogEvent(eventProperties); return true; } @@ -54,11 +65,22 @@ 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) { + // 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) { + 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 d052e7a9d..5ea0611e0 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( @@ -529,14 +531,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(); @@ -769,10 +774,17 @@ 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); - std::string key(utf); - env->ReleaseStringUTFChars(token, utf); - dropped[key] = static_cast(count); + 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. + if (utf != nullptr) + { + std::string key(utf); + env->ReleaseStringUTFChars(token, utf); + dropped[key] = static_cast(count); + } env.popLocalFrame(); } m_observer->OnStorageRecordsDropped(dropped); @@ -1098,8 +1110,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; } @@ -1343,7 +1358,13 @@ 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; + // 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, @@ -1359,14 +1380,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(); } diff --git a/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp b/lib/pal/universal/WindowsRuntimeSystemInformationImpl.cpp index 2ae7e9af4..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" @@ -80,7 +81,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); diff --git a/lib/system/TelemetrySystem.cpp b/lib/system/TelemetrySystem.cpp index 24ad34ba9..2e5059b47 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. 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 b2e34a99e..287e420ed 100644 --- a/tests/unittests/HttpClientManagerTests.cpp +++ b/tests/unittests/HttpClientManagerTests.cpp @@ -2,6 +2,7 @@ #include "common/Common.hpp" #include "common/MockIHttpClient.hpp" +#include "http/IBoundedHttpClientCancel.hpp" #include "http/HttpClientManager.hpp" #include "NullObjects.hpp" @@ -23,6 +24,11 @@ class HttpClientManager4Test : public HttpClientManager { { onHttpResponse(callback); } + + void setCancelDrainTimeout(std::chrono::milliseconds t) + { + m_cancelDrainTimeout = t; + } }; class HttpClientManagerTests : public StrictMock { @@ -42,6 +48,12 @@ class HttpClientManagerTests : public StrictMock { MOCK_METHOD1(resultRequestDone, void(EventsUploadContextPtr const &)); }; +class MockBoundedIHttpClient : public MockIHttpClient, public IBoundedHttpClientCancel { + public: + using MockIHttpClient::CancelAllRequests; + MOCK_METHOD1(CancelAllRequests, void(std::chrono::milliseconds)); +}; + TEST_F(HttpClientManagerTests, HandlesRequestFlow) { @@ -74,3 +86,73 @@ TEST_F(HttpClientManagerTests, HandlesRequestFlow) EXPECT_THAT(ctx->httpResponse, rspRef); EXPECT_THAT(ctx->durationMs, Gt(199)); } + +// 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) +{ + 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. + // The best-effort (pause) drain must still return, bounded by the drain timeout, + // rather than block forever. MockIHttpClient does not implement the bounded + // cancel capability, so HttpClientManager falls back to per-request async cancel + // and then abandons the drain when the callback remains outstanding. + EXPECT_CALL(httpClientMock, CancelRequestAsync(ctx->httpRequestId)).WillOnce(Return()); + auto start = std::chrono::steady_clock::now(); + hcm.cancelAllRequests(/* bestEffort */ true); + auto elapsedMs = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start).count(); + 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 + // safe to complete after cancelAllRequests abandoned the drain. + EXPECT_CALL(*this, resultRequestDone(ctx)).WillOnce(Return()); + callback->OnHttpResponse(new SimpleHttpResponse("stall")); +} + +TEST_F(HttpClientManagerTests, CancelAllRequests_UsesBoundedCancelCapability) +{ + MockBoundedIHttpClient boundedClient; + HttpClientManager4Test boundedHcm(boundedClient); + boundedHcm.setCancelDrainTimeout(std::chrono::milliseconds(150)); + boundedHcm.requestDone >> requestDone; + + SimpleHttpRequest* req = new SimpleHttpRequest("bounded"); + 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(boundedClient, SendRequestAsync(ctx->httpRequest, _)) + .WillOnce(SaveArg<1>(&callback)); + boundedHcm.sendRequest(ctx); + ASSERT_THAT(callback, NotNull()); + + EXPECT_CALL(boundedClient, CancelAllRequests(std::chrono::milliseconds(150))).WillOnce(Return()); + EXPECT_CALL(boundedClient, CancelRequestAsync(_)).Times(0); + + boundedHcm.cancelAllRequests(/* bestEffort */ true); + + EXPECT_CALL(*this, resultRequestDone(ctx)).WillOnce(Return()); + callback->OnHttpResponse(new SimpleHttpResponse("bounded")); +}