Default Win32 transport to WinHTTP with safe cancellation - #1515
Open
bmehta001 wants to merge 21 commits into
Open
Default Win32 transport to WinHTTP with safe cancellation#1515bmehta001 wants to merge 21 commits into
bmehta001 wants to merge 21 commits into
Conversation
WinInet is designed for interactive desktop apps: it depends on a logged-on user and that user's Internet Explorer settings, and Microsoft documents it as unsupported for services and other non-interactive processes. WinHTTP is Microsoft's own recommended replacement for exactly that scenario, and 1DS's dominant embedding scenario (background/service telemetry) is the one WinInet is not designed for. Add lib/http/HttpClient_WinHttp.hpp/.cpp implementing the same IHttpClient/IHttpRequest contract as HttpClient_WinInet using WinHTTP's async API instead. Key differences from a direct port of the WinInet implementation: - WinHttpOpen uses WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY (falling back to WINHTTP_ACCESS_TYPE_NO_PROXY on an older OS that rejects it) instead of WinInet's INTERNET_OPEN_TYPE_PRECONFIG, so proxy resolution does not require a logged-on user. - WinHTTP's async model has one distinct callback status per stage (SENDREQUEST_COMPLETE -> HEADERS_AVAILABLE -> DATA_AVAILABLE/READ_COMPLETE loop -> REQUEST_ERROR) rather than WinInet's single INTERNET_STATUS_REQUEST_COMPLETE, and a FALSE return from an async-handle call is always a genuine synchronous failure (never ERROR_IO_PENDING as with WinInet). - The response-size cap (MAX_HTTP_RESPONSE_SIZE, see microsoft#1508) is enforced the same way, before every read. - The MS-root certificate check rebuilds the chain via CertGetCertificateChain, since WinHttpQueryOption only hands back the leaf certificate rather than WinInet's ready-made chain context. - Request lifetime uses std::enable_shared_from_this / shared_ptr rather than raw-pointer self-ownership: WinHttpCloseHandle on a request with a pending operation blocks the calling thread until that operation's completion callback (which runs on a different WinHTTP-internal thread) finishes running. Holding the shared requests-map mutex across that call -- WinInet's pattern, safe there because its callback runs synchronously on the calling thread -- deadlocks here, since the callback thread needs that same mutex to erase() the completed request. shared_ptr lets cancellation release the map lock before the blocking close, while still safely keeping the wrapper alive against a concurrent natural completion. - CancelAllRequests() waits on a condition variable signaled from erase() instead of polling in a sleep loop. HttpClientFactory now selects WinHTTP by default on Win32 desktop (non-WinRT) builds. Set MATSDK_USE_WININET=ON (CMake) or define HAVE_MAT_WININET_HTTP_CLIENT (legacy MSBuild) to opt back into WinInet, e.g. for IE-integrated proxy/cookie behavior. Both cpp files are always compiled; the choice is made at the factory's #include/#ifdef site, matching the existing pattern for WinRt vs. WinInet. Wired into both build systems: lib/CMakeLists.txt (new source files, winhttp link library, MATSDK_USE_WININET option) and lib/pal/desktop/desktop.vcxitems (new source files; linking uses #pragma comment(lib, "winhttp.lib") in the new .cpp so no individual .vcxproj's AdditionalDependencies needs updating). Validation (Windows x64 Debug, both CMake and the Solutions\MSTelemetrySDK.sln MSBuild path actually used by CI): - UnitTests: 496/496 passed. - FuncTests: 43/43 passed, excluding sendManyRequestsAndCancel, which hits the real production collector over the internet. That specific test hangs identically with the original, unmodified WinInet client under the same back-to-back test sequence, confirming it is pre-existing network/infrastructure flakiness unrelated to this change, not a regression. - Found and fixed two real bugs during validation: (1) WinHttpSetStatusCallback's return value was checked as a boolean, when it actually returns the previous callback function pointer (typically null on first registration) -- this rejected every request immediately after registering the callback; (2) the deadlock described above, reproduced live via a hung sendManyRequestsAndCancel run and confirmed fixed by comparing CPU-active vs. CPU-static process state before and after the shared_ptr change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41
WinHTTP cancellation paths could leave the request wrapper in the parent map if HANDLE_CLOSING arrived without a prior terminal callback. That made CancelAllRequests wait forever and matched the Windows CI timeout in sendManyRequestsAndCancel. Handle HANDLE_CLOSING as a terminal signal when the request has not yet completed, so the wrapper erases itself and teardown always drains. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7fe5faca-d77c-45c4-85d3-0d4a00d68a94
Complete cancellation after WinHttpCloseHandle returns so HANDLE_CLOSING cannot dereference a destroyed request wrapper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Prevent detached UploadNow threads from outliving the functional test and racing later LogManager lifetimes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove completed requests before invoking application callbacks so concurrent teardown cannot destroy the wrapper while its terminal callback is still running. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ensure vcpkg-built Apple libraries match the consumer deployment target and avoid linker warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4259-b03b-8eeb87c06837
Propagate the resolved iOS sysroot to embedding builds and keep Apple vendored targets compatible with strict warning settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837
Remove legacy Apple architecture, platform, and deployment-target inputs so standalone scripts and embedding consumers share CMAKE_OSX_* configuration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837
Keep both selectable HTTP backends linked privately while dropping the unused Winsock dependency and headers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Baiju Meswani (baijumeswani)
approved these changes
Aug 7, 2026
bmehta001
enabled auto-merge (squash)
August 7, 2026 23:04
…ult-windows-transport
Join upload workers before SDK teardown and cover in-flight cancellation with a deterministic HTTP test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
…-transport' into bhamehta/winhttp-default-windows-transport
Preserve the WinHTTP default transport and WinInet opt-in while adopting the modern CMake build layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Route callbacks through a weak request reference so late WinHTTP notifications cannot dereference a destroyed wrapper during teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
bmehta001
changed the title
Avoid external collector network delays so teardown behavior is reproducible in CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Use a closed localhost port instead of creating hundreds of concurrent fixture connections. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MATSDK_USE_WININET=ONopt-in.Validation
BasicFuncTests.sendManyRequestsAndCancelreproduced across Win32/x64 Debug/Release and exposed the callback-lifetime race; the fix is pushed ind9522021.PR #1481 is Curl-only and does not overlap this WinHTTP change, so it was intentionally not merged or closed.