Default Win32 desktop transport to WinHTTP instead of WinInet - #1515
Open
bmehta001 wants to merge 8 commits into
Open
Default Win32 desktop transport to WinHTTP instead of WinInet#1515bmehta001 wants to merge 8 commits into
bmehta001 wants to merge 8 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
Prevent detached UploadNow threads from outliving the functional test and racing later LogManager lifetimes. Co-authored-by: Copilot App <[email protected]>
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 <[email protected]>
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
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.
Problem
HttpClient_WinInet(the default Win32 desktop HTTP transport) depends on alogged-on interactive user and that user's Internet Explorer proxy settings.
Microsoft documents WinInet as not supported for use in Windows services
or other non-interactive processes. Since 1DS's dominant real-world embedding
scenario is exactly that -- a background/service telemetry SDK -- WinInet is
the wrong default transport for most consumers.
Change
Add
lib/http/HttpClient_WinHttp.hpp/.cpp, a newIHttpClienttransportusing the WinHTTP API, and make it the default Win32 desktop (non-WinRT)
transport.
HttpClient_WinInetremains available as an explicit opt-in(
MATSDK_USE_WININET=ONvia CMake, orHAVE_MAT_WININET_HTTP_CLIENTfor thelegacy MSBuild solution) for callers that specifically need IE-integrated
proxy/cookie behavior.
Design notes (where this isn't just a mechanical port of WinInet)
WinHttpOpenusesWINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY(Windows 8.1+), which resolves the proxy without needing a logged-on user,
falling back to
WINHTTP_ACCESS_TYPE_NO_PROXYif an older OS rejects it.(
SENDREQUEST_COMPLETE->HEADERS_AVAILABLE->DATA_AVAILABLE/READ_COMPLETEloop ->
REQUEST_ERROR), unlike WinInet's singleINTERNET_STATUS_REQUEST_COMPLETE.A
FALSEreturn from an async-handle call is always a genuine synchronousfailure here (WinHTTP has no
ERROR_IO_PENDINGconvention like WinInet).MAX_HTTP_RESPONSE_SIZEDoS guard (Cap HTTP response body size across WinInet, WinRt, and Apple transports #1508) isenforced the same way, before every read.
CertGetCertificateChain,since
WinHttpQueryOptiononly returns the leaf certificate, unlikeWinInet's ready-made chain context option.
shared_ptr/enable_shared_from_thisratherthan WinInet's raw-pointer self-ownership.
WinHttpCloseHandleon arequest with a pending operation blocks the calling thread until that
operation's completion callback -- which runs on a different WinHTTP
worker thread -- finishes. 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_ptrlets cancellation release the map lock before the blockingclose while still safely keeping the wrapper alive against a concurrent
natural completion.
CancelAllRequests()waits on a condition variablesignaled from
erase()instead of polling in a sleep loop.Both
.cppfiles are always compiled; the transport choice is made atHttpClientFactory's#include/#ifdefsite, matching the existingWinRt-vs-WinInet pattern. Wired into both build systems:
lib/CMakeLists.txt(new sources,
winhttplink library,MATSDK_USE_WININEToption) andlib/pal/desktop/desktop.vcxitems(new sources; linking uses#pragma comment(lib, "winhttp.lib")in the new.cpp, so no individual.vcxproj'sAdditionalDependenciesneeds updating).Validation
Windows x64 Debug, both CMake and the
Solutions\MSTelemetrySDK.slnMSBuildpath actually used by CI (
build-tests.cmd/test-win-latest.yml):UnitTests: 496/496 passed.FuncTests: 43/43 passed, excludingsendManyRequestsAndCancel, whichhits the real production collector over the internet. That specific test
hangs identically with the original, unmodified
HttpClient_WinInetunder the same back-to-back test sequence -- confirmed by direct
comparison -- so it is pre-existing network/infrastructure flakiness
unrelated to this change, not a regression.
WinHttpSetStatusCallback's return value was checked as a boolean, whenit actually returns the previous callback function pointer (typically
null on first registration) -- this rejected every request immediately
after registering the callback. Confirmed by a targeted functional test
going from failing (0 requests ever reaching a local test server) to
passing.
WinHttpCloseHandlecross-thread deadlock described above,reproduced live via a hung
sendManyRequestsAndCancelrun (static CPU,blocked threads) and confirmed fixed by the same test completing in
~18s after the
shared_ptrchange.