diff --git a/.github/workflows/build-ios-mac.yml b/.github/workflows/build-ios-mac.yml index 29b3dfc34..fd3eba761 100644 --- a/.github/workflows/build-ios-mac.yml +++ b/.github/workflows/build-ios-mac.yml @@ -61,8 +61,8 @@ jobs: - name: build run: | if [[ "${{ matrix.os }}" == "macos-14" ]]; then - export IOS_DEPLOYMENT_TARGET=13.0; + export CMAKE_OSX_DEPLOYMENT_TARGET=13.0; elif [[ "${{ matrix.os }}" == "macos-15" ]]; then - export IOS_DEPLOYMENT_TARGET=15.0; + export CMAKE_OSX_DEPLOYMENT_TARGET=15.0; fi ./build-tests-ios.sh ${{ matrix.config }} ${{ matrix.simulator }} diff --git a/CMakeLists.txt b/CMakeLists.txt index cc36e9da3..69785b37c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -47,88 +47,42 @@ if(APPLE) message(STATUS "BUILD_IOS: ${BUILD_IOS}") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fobjc-arc") - # iOS build options - option(BUILD_IOS "Build for iOS" NO) - option(FORCE_RESET_OSX_DEPLOYMENT_TARGET "Clear the OSX Deployment Target Set" YES) - if (DEFINED FORCE_RESET_DEPLOYMENT_TARGET) - set(FORCE_RESET_OSX_DEPLOYMENT_TARGET ${FORCE_RESET_DEPLOYMENT_TARGET}) - endif() + option(BUILD_IOS "Build for iOS-family Apple platforms" NO) # When building via vcpkg, the toolchain file handles architecture, sysroot, # deployment target, and platform flags. Skip manual flag configuration. if(NOT MATSDK_USE_VCPKG_DEPS) + if(CMAKE_SYSTEM_NAME MATCHES "^(iOS|visionOS)$") + set(BUILD_IOS ON) + endif() if(BUILD_IOS) set(TARGET_ARCH "APPLE") - set(IOS True) set(APPLE True) - if(FORCE_RESET_OSX_DEPLOYMENT_TARGET) - set(CMAKE_OSX_DEPLOYMENT_TARGET "" CACHE STRING "Force unset of the deployment target for iOS" FORCE) - if (${IOS_PLAT} STREQUAL "iphonesimulator") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -mios-simulator-version-min=${IOS_DEPLOYMENT_TARGET}") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -mios-simulator-version-min=${IOS_DEPLOYMENT_TARGET}") - else() - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -miphoneos-version-min=${IOS_DEPLOYMENT_TARGET}") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -miphoneos-version-min=${IOS_DEPLOYMENT_TARGET}") - endif() - endif() - - if((${IOS_PLAT} STREQUAL "iphoneos") OR (${IOS_PLAT} STREQUAL "iphonesimulator") OR (${IOS_PLAT} STREQUAL "xros") OR (${IOS_PLAT} STREQUAL "xrsimulator")) - set(IOS_PLATFORM "${IOS_PLAT}") - else() - message(FATAL_ERROR "Unrecognized iOS platform '${IOS_PLAT}'") + if(NOT CMAKE_OSX_SYSROOT) + message(FATAL_ERROR "CMAKE_OSX_SYSROOT must identify an Apple SDK") endif() - - if(${IOS_ARCH} STREQUAL "x86_64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch x86_64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch x86_64") - set(CMAKE_SYSTEM_PROCESSOR x86_64) - elseif(${IOS_ARCH} STREQUAL "arm64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch arm64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch arm64") - set(CMAKE_SYSTEM_PROCESSOR arm64) - elseif(${IOS_ARCH} STREQUAL "arm64e") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch arm64e") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch arm64e") - set(CMAKE_SYSTEM_PROCESSOR arm64e) - else() - message(FATAL_ERROR "Unrecognized iOS architecture '${IOS_ARCH}'") - endif() - - execute_process(COMMAND xcodebuild -version -sdk ${IOS_PLATFORM} ONLY_ACTIVE_ARCH=NO Path + if(NOT IS_ABSOLUTE "${CMAKE_OSX_SYSROOT}") + execute_process(COMMAND xcodebuild -version -sdk "${CMAKE_OSX_SYSROOT}" Path OUTPUT_VARIABLE CMAKE_OSX_SYSROOT ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) + if(NOT CMAKE_OSX_SYSROOT) + message(FATAL_ERROR "Unable to resolve the Apple SDK sysroot") + endif() + set(CMAKE_OSX_SYSROOT "${CMAKE_OSX_SYSROOT}" CACHE PATH "Apple SDK sysroot" FORCE) + endif() message(STATUS "CMAKE_OSX_SYSROOT ${CMAKE_OSX_SYSROOT}") message(STATUS "ARCHITECTURE: ${CMAKE_SYSTEM_PROCESSOR}") - message(STATUS "PLATFORM: ${IOS_PLATFORM}") + message(STATUS "DEPLOYMENT TARGET: ${CMAKE_OSX_DEPLOYMENT_TARGET}") else() - if("${MAC_ARCH}" STREQUAL "x86_64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch x86_64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch x86_64") - set(CMAKE_SYSTEM_PROCESSOR x86_64) - set(TARGET_ARCH ${CMAKE_SYSTEM_PROCESSOR}) - set(CMAKE_OSX_ARCHITECTURES ${MAC_ARCH}) - set(APPLE True) - elseif("${MAC_ARCH}" STREQUAL "arm64") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch arm64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch arm64") - set(CMAKE_SYSTEM_PROCESSOR arm64) - set(TARGET_ARCH ${CMAKE_SYSTEM_PROCESSOR}) - set(CMAKE_OSX_ARCHITECTURES ${MAC_ARCH}) - set(APPLE True) - else() - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -arch x86_64 -arch arm64") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -arch x86_64 -arch arm64") - endif() - message(STATUS "MAC_ARCH: ${MAC_ARCH}") + message(STATUS "ARCHITECTURES: ${CMAKE_OSX_ARCHITECTURES}") endif() else() # vcpkg mode: just set internal flags from what the toolchain provides - if(BUILD_IOS OR CMAKE_SYSTEM_NAME STREQUAL "iOS") + if(BUILD_IOS OR CMAKE_SYSTEM_NAME MATCHES "^(iOS|visionOS)$") set(BUILD_IOS ON) set(TARGET_ARCH "APPLE") - set(IOS True) endif() message(STATUS "vcpkg toolchain managing architecture and platform flags") endif() diff --git a/build-gtest.sh b/build-gtest.sh index 4c73f3382..4dca08f06 100755 --- a/build-gtest.sh +++ b/build-gtest.sh @@ -39,9 +39,8 @@ if(BUILD_IOS) set(CMAKE_OSX_DEPLOYMENT_TARGET "12.2" CACHE STRING "Force set of the deployment target for iOS" FORCE) set(CMAKE_C_FLAGS "\${CMAKE_C_FLAGS} -miphoneos-version-min=10.0") set(CMAKE_CXX_FLAGS "\${CMAKE_CXX_FLAGS} -miphoneos-version-min=10.0 -std=c++11") - set(IOS_PLATFORM "iphonesimulator") set(CMAKE_SYSTEM_PROCESSOR x86_64) - execute_process(COMMAND xcodebuild -version -sdk \${IOS_PLATFORM} Path + execute_process(COMMAND xcodebuild -version -sdk iphonesimulator Path OUTPUT_VARIABLE CMAKE_OSX_SYSROOT_OUT ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) diff --git a/build-ios.sh b/build-ios.sh index d316fe2fa..be53816e2 100755 --- a/build-ios.sh +++ b/build-ios.sh @@ -25,51 +25,47 @@ elif [ "$1" == "debug" ]; then fi # Set Architecture: arm64, arm64e or x86_64 -IOS_ARCH=$(/usr/bin/uname -m) +APPLE_ARCH=$(/usr/bin/uname -m) if [ "$1" == "arm64" ]; then - IOS_ARCH="arm64" + APPLE_ARCH="arm64" shift elif [ "$1" == "arm64e" ]; then - IOS_ARCH="arm64e" + APPLE_ARCH="arm64e" shift elif [ "$1" == "x86_64" ]; then - IOS_ARCH="x86_64" + APPLE_ARCH="x86_64" shift fi # the last param is expected to specify the platform name: iphoneos|iphonesimulator|xros|xrsimulator # so if it is non-empty and it is not "device", we take it as a valid platform name # otherwise we fall back to old iOS logic which only supported iphoneos|iphonesimulator -IOS_PLAT="iphonesimulator" +APPLE_PLATFORM="iphonesimulator" if [ -n "$1" ] && [ "$1" != "device" ]; then - IOS_PLAT="$1" + APPLE_PLATFORM="$1" elif [ "$1" == "device" ]; then - IOS_PLAT="iphoneos" + APPLE_PLATFORM="iphoneos" fi -echo "IOS_ARCH = $IOS_ARCH, IOS_PLAT = $IOS_PLAT, BUILD_TYPE = $BUILD_TYPE" +echo "architecture = $APPLE_ARCH, platform = $APPLE_PLATFORM, build type = $BUILD_TYPE" -FORCE_RESET_DEPLOYMENT_TARGET=NO DEPLOYMENT_TARGET="" -if [ "$IOS_PLAT" == "iphoneos" ] || [ "$IOS_PLAT" == "iphonesimulator" ]; then +if [ "$APPLE_PLATFORM" == "iphoneos" ] || [ "$APPLE_PLATFORM" == "iphonesimulator" ]; then SYS_NAME="iOS" - DEPLOYMENT_TARGET="$IOS_DEPLOYMENT_TARGET" + DEPLOYMENT_TARGET="$CMAKE_OSX_DEPLOYMENT_TARGET" if [ -z "$DEPLOYMENT_TARGET" ]; then DEPLOYMENT_TARGET="12.0" - FORCE_RESET_DEPLOYMENT_TARGET=YES fi -elif [ "$IOS_PLAT" == "xros" ] || [ "$IOS_PLAT" == "xrsimulator" ]; then +elif [ "$APPLE_PLATFORM" == "xros" ] || [ "$APPLE_PLATFORM" == "xrsimulator" ]; then SYS_NAME="visionOS" - DEPLOYMENT_TARGET="$XROS_DEPLOYMENT_TARGET" + DEPLOYMENT_TARGET="$CMAKE_OSX_DEPLOYMENT_TARGET" if [ -z "$DEPLOYMENT_TARGET" ]; then DEPLOYMENT_TARGET="1.0" - FORCE_RESET_DEPLOYMENT_TARGET=YES fi fi echo "deployment target = $DEPLOYMENT_TARGET" -echo "force reset deployment target = $FORCE_RESET_DEPLOYMENT_TARGET" # Install build tools and recent sqlite3 FILE=".buildtools" @@ -92,7 +88,7 @@ cd out CMAKE_PACKAGE_TYPE=tgz -cmake_cmd="cmake -DCMAKE_OSX_SYSROOT=$IOS_PLAT -DCMAKE_SYSTEM_NAME=$SYS_NAME -DCMAKE_IOS_ARCH_ABI=$IOS_ARCH -DCMAKE_OSX_DEPLOYMENT_TARGET=$DEPLOYMENT_TARGET -DBUILD_IOS=YES -DIOS_ARCH=$IOS_ARCH -DIOS_PLAT=$IOS_PLAT -DIOS_DEPLOYMENT_TARGET=$DEPLOYMENT_TARGET -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE -DFORCE_RESET_DEPLOYMENT_TARGET=$FORCE_RESET_DEPLOYMENT_TARGET $CMAKE_OPTS .." +cmake_cmd="cmake -DCMAKE_OSX_SYSROOT=$APPLE_PLATFORM -DCMAKE_SYSTEM_NAME=$SYS_NAME -DCMAKE_OSX_ARCHITECTURES=$APPLE_ARCH -DCMAKE_OSX_DEPLOYMENT_TARGET=$DEPLOYMENT_TARGET -DBUILD_IOS=YES -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE $CMAKE_OPTS .." echo "${cmake_cmd}" eval $cmake_cmd diff --git a/build-tests.cmd b/build-tests.cmd index e6dc4bf6a..7f3d0a0ba 100644 --- a/build-tests.cmd +++ b/build-tests.cmd @@ -53,12 +53,10 @@ set MAXCPUCOUNT=%NUMBER_OF_PROCESSORS% set SOLUTION=Solutions\MSTelemetrySDK.sln msbuild %SOLUTION% /target:sqlite:Rebuild,zlib:Rebuild,Tests\gmock:Rebuild,Tests\gtest:Rebuild,Tests\UnitTests:Rebuild,Tests\FuncTests:Rebuild /p:BuildProjectReferences=true /maxcpucount:%MAXCPUCOUNT% /detailedsummary /p:Configuration=%CONFIGURATION% /p:Platform=%PLAT% %CUSTOM_PROPS% -if errorLevel 1 goto end +if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL% Solutions\out\%CONFIGURATION%\%PLAT%\UnitTests\UnitTests.exe -if errorLevel 1 goto end +if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL% Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe -:end -if errorLevel 1 goto end -start "" Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe --gtest_filter=MultipleLogManagersTests.MultiProcessesLogManager -start "" Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe --gtest_filter=MultipleLogManagersTests.MultiProcessesLogManager -:end +if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL% +powershell -NoProfile -ExecutionPolicy Bypass -Command "$path = Join-Path (Get-Location) 'Solutions\out\%CONFIGURATION%\%PLAT%\FuncTests\FuncTests.exe'; $args = '--gtest_filter=MultipleLogManagersTests.MultiProcessesLogManager'; $p1 = Start-Process -FilePath $path -ArgumentList $args -PassThru; $p2 = Start-Process -FilePath $path -ArgumentList $args -PassThru; $p1.WaitForExit(); $p2.WaitForExit(); if ($p1.ExitCode -ne 0 -or $p2.ExitCode -ne 0) { exit 1 }" +if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL% diff --git a/build.sh b/build.sh index 52a5081b2..07dd19a6f 100755 --- a/build.sh +++ b/build.sh @@ -61,13 +61,13 @@ while [[ $# -gt 0 ]]; do echo "BUILD_TYPE = $BUILD_TYPE" ;; arm64|x86_64|universal) - if [[ -n "$MAC_ARCH" ]]; then - echo "Error: MAC_ARCH is already set to '$MAC_ARCH'. Cannot overwrite with $ARG." 1>&2 + if [[ -n "$APPLE_ARCH" ]]; then + echo "Error: APPLE_ARCH is already set to '$APPLE_ARCH'. Cannot overwrite with $ARG." 1>&2 exit 1 else - MAC_ARCH="$ARG" + APPLE_ARCH="$ARG" fi - echo "MAC_ARCH = $MAC_ARCH" + echo "APPLE_ARCH = $APPLE_ARCH" ;; CUSTOM_BUILD_FLAGS*) CUSTOM_CMAKE_CXX_FLAG="\"${ARG:19:999}\"" @@ -91,9 +91,9 @@ if [[ -z "$BUILD_TYPE" ]]; then echo "Assuming default BUILD_TYPE = Debug" fi -if [[ -z "$MAC_ARCH" ]]; then - MAC_ARCH=$(/usr/bin/uname -m) - echo "Using current machine MAC_ARCH = $MAC_ARCH" +if [[ -z "$APPLE_ARCH" ]]; then + APPLE_ARCH=$(/usr/bin/uname -m) + echo "Using current machine APPLE_ARCH = $APPLE_ARCH" fi # Evaluate switches @@ -137,7 +137,7 @@ if [ "$LINK_TYPE" == "shared" ]; then fi # Set target MacOS minver -default_mac_os_target=$([ "$MAC_ARCH" == "arm64" ] && echo "11.10" || echo "10.10") +default_mac_os_target=$([ "$APPLE_ARCH" == "arm64" ] && echo "11.10" || echo "10.10") [ -z $MACOSX_DEPLOYMENT_TARGET ] && export MACOSX_DEPLOYMENT_TARGET=${default_mac_os_target} echo "macosx deployment target="$MACOSX_DEPLOYMENT_TARGET @@ -147,7 +147,7 @@ OS_NAME=`uname -a` if [ ! -f $FILE ]; then case "$OS_NAME" in - *Darwin*) CMD="tools/setup-buildtools-apple.sh $MAC_ARCH" ;; + *Darwin*) CMD="tools/setup-buildtools-apple.sh $APPLE_ARCH" ;; *Linux*) CMD="tools/setup-buildtools.sh" ;; *) CMD=""; echo "WARNING: unsupported OS $OS_NAME, skipping build tools installation.." ;; esac @@ -185,8 +185,7 @@ fi # Fail on error set -e -# TODO: should this be improved to verify if the platform is Apple? Right now we unconditionally pass -DMAC_ARCH even if building for Windows or Linux. -cmake_cmd="cmake -DMAC_ARCH=$MAC_ARCH -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE -DCMAKE_CXX_FLAGS="${CUSTOM_CMAKE_CXX_FLAG}" $CMAKE_OPTS .." +cmake_cmd="cmake -DCMAKE_OSX_ARCHITECTURES=$APPLE_ARCH -DCMAKE_OSX_DEPLOYMENT_TARGET=$MACOSX_DEPLOYMENT_TARGET -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_PACKAGE_TYPE=$CMAKE_PACKAGE_TYPE -DCMAKE_CXX_FLAGS="${CUSTOM_CMAKE_CXX_FLAG}" $CMAKE_OPTS .." echo $cmake_cmd eval $cmake_cmd diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index 13b4d46d4..994fbf9b2 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -490,7 +490,12 @@ if(MATSDK_BUNDLE_SQLITE AND NOT TARGET sqlite3_bundled) else() # Unstripped vendored build (Android legacy): keep the existing narrower # warning suppression. -fno-finite-math-only guards the INFINITY macro. - target_compile_options(sqlite3_bundled PRIVATE -fno-finite-math-only -Wno-unused-function) + target_compile_options(sqlite3_bundled PRIVATE + -fno-finite-math-only + -Wno-unused-function + -Wno-shorten-64-to-32 + -Wno-ambiguous-macro + ) endif() endif() @@ -561,6 +566,10 @@ else() # real POSIX declarations for read/write/lseek/close instead of relying on # implicit (int-returning) declarations. target_compile_definitions(zlib_bundled PRIVATE Z_HAVE_UNISTD_H) + target_compile_options(zlib_bundled PRIVATE + -Wno-shorten-64-to-32 + -Wno-ambiguous-macro + ) target_link_libraries(mat PRIVATE sqlite3_bundled zlib_bundled ${LIBS}) elseif(PAL_IMPLEMENTATION STREQUAL "WIN32") diff --git a/lib/http/HttpClientManager.cpp b/lib/http/HttpClientManager.cpp index 0de14e085..3c7d1f809 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,90 @@ namespace MAT_NS_BEGIN { LOG_TRACE("HTTP remove callback=%p", callback); m_httpCallbacks.remove(callback); + // Wake cancelAllRequests() waiting for the list to drain. + m_httpCallbacksCV.notify_all(); } delete callback; } - bool HttpClientManager::cancelAllRequestsAsync() + void HttpClientManager::cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout) { + if (bestEffortTimeout > std::chrono::milliseconds::zero()) + { +#if defined(_CPPRTTI) || defined(__GXX_RTTI) + auto boundedCancel = dynamic_cast(&m_httpClient); + if (boundedCancel != nullptr) + { + boundedCancel->CancelAllRequests(bestEffortTimeout); + return; + } +#endif + + cancelTrackedRequestsAsync(); + return; + } + m_httpClient.CancelAllRequests(); - return true; } - void HttpClientManager::cancelAllRequests() + void HttpClientManager::cancelTrackedRequestsAsync() { - cancelAllRequestsAsync(); - - // Wait for callbacks to drain before shutdown can destroy state that - // those callbacks still use. Keep the list check synchronized and sleep - // between polls so a slow adapter does not burn CPU while draining. - for (;;) + std::vector requestIds; { + LOCKGUARD(m_httpCallbacksMtx); + for (const auto& callback : m_httpCallbacks) { - LOCKGUARD(m_httpCallbacksMtx); - if (m_httpCallbacks.empty()) + if (callback == nullptr || callback->m_ctx == nullptr) { - return; + continue; + } + + std::string id = callback->m_ctx->httpRequestId; + if (id.empty() && callback->m_ctx->httpRequest != nullptr) + { + id = callback->m_ctx->httpRequest->GetId(); + } + if (!id.empty()) + { + requestIds.push_back(id); } } - std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + + for (const auto& id : requestIds) + { + m_httpClient.CancelRequestAsync(id); + } + } + + void HttpClientManager::cancelAllRequests(bool bestEffort) + { + // Use the transport-specific bounded path when available; older clients + // fall back to cancelling tracked requests individually. + const auto cancelStart = std::chrono::steady_clock::now(); + cancelAllRequestsAsync(bestEffort ? m_cancelDrainTimeout : std::chrono::milliseconds::zero()); + + // Drain callbacks through the condition variable signaled by onHttpResponse. + std::unique_lock lock(m_httpCallbacksMtx); + if (bestEffort) + { + // Keep pause bounded, including time spent in the transport cancel. + 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 is the lifetime barrier for callback state, so drain fully. + m_httpCallbacksCV.wait(lock, [this] { return m_httpCallbacks.empty(); }); } } diff --git a/lib/http/HttpClientManager.hpp b/lib/http/HttpClientManager.hpp index e8214d631..4f350e37f 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,9 @@ class HttpClientManager virtual ~HttpClientManager() noexcept; - void cancelAllRequests(); + // Cancel in-flight requests. Shutdown drains fully; pause uses a bounded, + // best-effort drain because it may run under the LogManager lock. + void cancelAllRequests(bool bestEffort = false); size_t requestCount() const { @@ -55,14 +59,22 @@ class HttpClientManager void handleSendRequest(EventsUploadContextPtr const& ctx); virtual void scheduleOnHttpResponse(HttpCallback* callback); void onHttpResponse(HttpCallback* callback); - bool cancelAllRequestsAsync(); + void cancelAllRequestsAsync(std::chrono::milliseconds bestEffortTimeout = std::chrono::milliseconds::zero()); + void cancelTrackedRequestsAsync(); ILogManager& m_logManager; IHttpClient& m_httpClient; ITaskDispatcher& m_taskDispatcher; mutable std::recursive_mutex m_httpCallbacksMtx; std::list 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_Apple.mm b/lib/http/HttpClient_Apple.mm index b7d6646a4..1a047f5d6 100644 --- a/lib/http/HttpClient_Apple.mm +++ b/lib/http/HttpClient_Apple.mm @@ -207,7 +207,7 @@ void HandleResponse(NSData* data, NSURLResponse* response, NSError* error) NSHTTPURLResponse *httpResp = static_cast(response); auto simpleResponse = new SimpleHttpResponse { NextRespId() }; - simpleResponse->m_statusCode = httpResp.statusCode; + simpleResponse->m_statusCode = static_cast(httpResp.statusCode); NSDictionary *responseHeaders = [httpResp allHeaderFields]; for (id key in responseHeaders) diff --git a/lib/http/HttpClient_WinInet.cpp b/lib/http/HttpClient_WinInet.cpp index b1d3b4013..2ec8be9b0 100644 --- a/lib/http/HttpClient_WinInet.cpp +++ b/lib/http/HttpClient_WinInet.cpp @@ -503,6 +503,8 @@ void HttpClient_WinInet::erase(std::string const& id) if (it != m_requests.end()) { auto req = it->second; m_requests.erase(it); + // Wake CancelAllRequests() waiting for the map to drain. + m_requestsCV.notify_all(); // delete WinInetRequestWrapper delete req; } @@ -535,6 +537,11 @@ void HttpClient_WinInet::CancelRequestAsync(std::string const& id) void HttpClient_WinInet::CancelAllRequests() +{ + CancelAllRequests(std::chrono::milliseconds::zero()); +} + +void HttpClient_WinInet::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { // vector of all request IDs std::vector ids; @@ -548,11 +555,26 @@ void HttpClient_WinInet::CancelAllRequests() for (const auto &id : ids) CancelRequestAsync(id); - // wait for all destructors to run - while (!m_requests.empty()) + // Wait for all request destructors to run (erase() removes them on the WinInet + // callback thread). Use a condition variable signaled from erase() rather than a + // poll loop so this never spins at 100% CPU while draining. WinInet delivers the + // cancellation callbacks on its own threads, so the wait completes without + // depending on the SDK task dispatcher. + std::unique_lock 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 1efc1bb22..12ac6aa00 100644 --- a/lib/http/HttpClient_WinRt.cpp +++ b/lib/http/HttpClient_WinRt.cpp @@ -399,6 +399,11 @@ namespace MAT_NS_BEGIN { } void HttpClient_WinRt::CancelAllRequests() + { + CancelAllRequests(std::chrono::milliseconds::zero()); + } + + void HttpClient_WinRt::CancelAllRequests(std::chrono::milliseconds bestEffortTimeout) { // vector of all request IDs std::vector ids; @@ -412,11 +417,40 @@ namespace MAT_NS_BEGIN { for (const auto &id : ids) CancelRequestAsync(id); - // wait for all destructors to run - while (!m_requests.empty()) + // wait for all destructors to run. Read m_requests under the lock each + // iteration; erase() runs on the PPL continuation thread under the same lock. + // A zero timeout drains fully (shutdown); a positive timeout is a best-effort + // cap so callers such as pause do not block indefinitely. + const bool bounded = bestEffortTimeout > std::chrono::milliseconds::zero(); + const auto deadline = std::chrono::steady_clock::now() + bestEffortTimeout; + bool done; { - PAL::sleep(100); + std::lock_guard 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 7a8678ceb..0b2727803 100644 --- a/lib/include/public/IHttpClient.hpp +++ b/lib/include/public/IHttpClient.hpp @@ -556,6 +556,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() {} /// @@ -572,4 +576,3 @@ namespace MAT_NS_BEGIN } MAT_NS_END #endif - diff --git a/lib/jni/JniConvertors.cpp b/lib/jni/JniConvertors.cpp index d944ddff8..dc61a4304 100644 --- a/lib/jni/JniConvertors.cpp +++ b/lib/jni/JniConvertors.cpp @@ -13,6 +13,11 @@ std::string JStringToStdString(JNIEnv* env, const jstring& jstr) { size_t jstr_length = env->GetStringUTFLength(jstr); auto jstr_utf = env->GetStringUTFChars(jstr, nullptr); + if (jstr_utf == nullptr) { + // Preserve the pending Java exception (typically an allocation failure) + // so the JNI caller observes the real failure instead of an empty value. + return ""; + } std::string str(jstr_utf, jstr_utf + jstr_length); env->ReleaseStringUTFChars(jstr, jstr_utf); return str; @@ -160,7 +165,7 @@ EventProperties GetEventProperties(JNIEnv* env, const jstring& jstrEventName, co EventProperties eventProperties; eventProperties.SetName(JStringToStdString(env, jstrEventName)); if (jstrEventType != NULL) { - // An empty type means "unset" (the native default). Before #1329 the + // An empty type means "unset" (the native default). Previously the // Java getType() returned null for a default EventProperties, so this // branch was skipped. getType() now returns "" to fix a Java-side NPE; // forwarding SetType("") here would fail native event-name validation @@ -208,4 +213,3 @@ std::vector ConvertJObjectArrayToStdStringVector(JNIEnv* env, const } } MAT_NS_END - diff --git a/lib/jni/Signals_jni.cpp b/lib/jni/Signals_jni.cpp index 59ca6f32d..b16a48c8d 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; + auto logger = reinterpret_cast(nativeLoggerPtr); + if (logger == nullptr) { + return false; + } + if (signal_item_json == nullptr) { + return false; + } const char *signalItemJson = (env)->GetStringUTFChars(signal_item_json, &isCopy); - env->ReleaseStringUTFChars(signal_item_json, signalItemJson); + if (signalItemJson == nullptr) { + // Preserve the pending Java exception (typically an allocation failure) + // rather than masking it as a clean false result. + return false; + } - auto logger = reinterpret_cast(nativeLoggerPtr); EventProperties eventProperties = Signals::CreateEventProperties(signalItemJson); + env->ReleaseStringUTFChars(signal_item_json, signalItemJson); logger->LogEvent(eventProperties); return true; } @@ -54,20 +65,34 @@ Java_com_microsoft_applications_events_Signals_nativeInitialize(JNIEnv *env, jcl SubstrateSignalsConfiguration config; jboolean isCopy = true; - const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy); - if (strlen(convertedValue) > 0) { - config.ServiceRequestConfig.BaseUrl = convertedValue; + if (base_url != nullptr) { + const char *convertedValue = (env)->GetStringUTFChars(base_url, &isCopy); + if (convertedValue == nullptr) { + // Preserve the pending Java exception (typically an allocation failure) + // rather than masking it as a clean false result. + return false; + } + if (strlen(convertedValue) > 0) { + config.ServiceRequestConfig.BaseUrl = convertedValue; + } + env->ReleaseStringUTFChars(base_url, convertedValue); } - env->ReleaseStringUTFChars(base_url, convertedValue); config.ServiceRequestConfig.TimeoutMs = reinterpret_cast(timeout_ms); config.ServiceRequestConfig.RetryTimes = reinterpret_cast(retry_times); config.ServiceRequestConfig.RetryTimesToWait = reinterpret_cast(retry_time_to_wait); - jsize size = env->GetArrayLength(retry_status_codes); - std::vector retryStatusCodes(size); - env->GetIntArrayRegion(retry_status_codes, jsize{0}, size, &retryStatusCodes[0] ); - config.ServiceRequestConfig.RetryStatusCodes = std::vector(retryStatusCodes.begin(), retryStatusCodes.end()); + if (retry_status_codes != nullptr) + { + jsize size = env->GetArrayLength(retry_status_codes); + std::vector retryStatusCodes(size); + if (size > 0) + { + env->GetIntArrayRegion(retry_status_codes, jsize{0}, size, retryStatusCodes.data()); + } + config.ServiceRequestConfig.RetryStatusCodes = + std::vector(retryStatusCodes.begin(), retryStatusCodes.end()); + } spDataInspector = Signals::CreateSignalsEventInspector(nullptr, config); return true; diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index d5f5a1211..244edc537 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -32,14 +32,14 @@ namespace MAT_NS_BEGIN { } KillSwitchManager() - : KillSwitchManager([]() { return static_cast(PAL::getMonotonicTimeMs()); }) + : KillSwitchManager(defaultClock()) { } explicit KillSwitchManager(Clock clock) : m_clock(clock ? std::move(clock) - : Clock([]() { return static_cast(PAL::getMonotonicTimeMs()); })), + : defaultClock()), m_isRetryAfterActive(false), m_retryAfterExpiryTime(0) { @@ -186,6 +186,11 @@ namespace MAT_NS_BEGIN { } private: + static Clock defaultClock() + { + return []() { return static_cast(PAL::getMonotonicTimeMs()); }; + } + // Precondition: seconds > 0. All call sites enforce this (handleResponse // and addToken both guard with `timeinSecs > 0` / `timeInSeconds > 0`). // Passing a non-positive value is UB: a negative durationMs makes the 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..fa7ac6575 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,19 @@ 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; + } + catch (Platform::Exception^) + { + versionDec = 0ull; + } if (versionDec != 0ull) { m_os_major_version = std::to_string(versionDec >> 16 * 3) + "." + std::to_string(versionDec >> 16 * 2 & 0xFFFF); @@ -129,4 +142,3 @@ namespace PAL_NS_BEGIN { } } PAL_NS_END - 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")); +} diff --git a/tools/ports/cpp-client-telemetry/portfile.cmake b/tools/ports/cpp-client-telemetry/portfile.cmake index b2fdab830..011c1c1f0 100644 --- a/tools/ports/cpp-client-telemetry/portfile.cmake +++ b/tools/ports/cpp-client-telemetry/portfile.cmake @@ -46,6 +46,14 @@ if(VCPKG_TARGET_IS_IOS) set(MATSDK_BUILD_IOS ON) endif() +# Keep the port's iOS deployment target aligned with the consumer test and the +# SDK's supported minimum instead of letting Clang default to the SDK version. +set(MATSDK_APPLE_DEPLOYMENT_OPTIONS) +if(VCPKG_TARGET_IS_IOS) + list(APPEND MATSDK_APPLE_DEPLOYMENT_OPTIONS + -DCMAKE_OSX_DEPLOYMENT_TARGET=12.0) +endif() + set(MATSDK_ANDROID_HTTP_CLIENT AUTO) if(VCPKG_TARGET_IS_ANDROID) file(READ "${SOURCE_PATH}/CMakeLists.txt" _matsdk_root_cmake) @@ -131,6 +139,7 @@ vcpkg_cmake_configure( -DBUILD_VERSION=${VERSION} -DBUILD_APPLE_HTTP=${MATSDK_BUILD_APPLE_HTTP} -DBUILD_IOS=${MATSDK_BUILD_IOS} + ${MATSDK_APPLE_DEPLOYMENT_OPTIONS} ) vcpkg_cmake_install()