Add PLM suspend and request-limit features to WinHTTP, removing XCurl GDK path - #1013
Open
Jason Sandlin (jasonsandlin) wants to merge 7 commits into
Open
Add PLM suspend and request-limit features to WinHTTP, removing XCurl GDK path#1013Jason Sandlin (jasonsandlin) wants to merge 7 commits into
Jason Sandlin (jasonsandlin) wants to merge 7 commits into
Conversation
RegisterAppStateChangeNotification was called under RETURN_IF_FAILED, so a failure to subscribe took down HTTP entirely. That API resolves from api-ms-win-core-psm-appnotify, which is not guaranteed to be present in every configuration the GDK runs in, and suspend handling is a resilience feature rather than a prerequisite for making requests. The CurlProvider path this change set replaces already soft-failed here; this restores that behavior for WinHTTP. On failure we log, leave the token null, and continue without suspend notifications. The destructor already tolerates a null token and Suspend/Resume simply never fire. Addresses PR #1013 review feedback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d849a6b-2837-4f03-9b04-48dc22e535b4
CloseAllConnections cleared m_connections up front, so any connection that failed to report closed within the bounded suspend wait was dropped from tracking entirely. Shutdown then had nothing to wait on for those stragglers, which defeats the INFINITE wait on the teardown path: the destructor could go on to close the WinHTTP session handles a still-live connection was using. Re-closing them is not an option. WinHttpConnection::Close is once-only and returns E_UNEXPECTED on a second call without ever invoking the callback, so a naive retry under INFINITE would hang forever. Instead, retain the close context those connections were already given and wait on it from a later call. That drain is deliberately bounded even when the caller passed INFINITE: a connection that already missed one deadline is the one least likely to ever report, and blocking HCCleanup forever would be worse than the race it closes. The unbounded wait still applies to connections closed by the current call, which is the case that actually protects the session handles. Addresses PR #1013 review feedback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d849a6b-2837-4f03-9b04-48dc22e535b4
Scott Munro (ScottMunroMS)
left a comment
Scott Munro (ScottMunroMS)
left a comment
Contributor
There was a problem hiding this comment.
some food for thought. no hard blockers
- Scope the default request limit to Xbox consoles. It is resolved at runtime via XSystemGetDeviceType because the same Gaming.Desktop binary ships on Xbox, GDK PC, Steam and Steam Deck. Those PC devices were never throttled before this change, so they stay unlimited unless a title opts in. - Return E_NOTIMPL from HCSettingsSet/GetGlobalRequestLimit on platforms whose HTTP provider implements no admission control, rather than storing a value that would silently never throttle anything. - Append the new exports to the end of all four .def files so existing ordinals are not shifted, matching the repo's additive-ABI convention. - Gate the retained-connection drain to shutdown only; suspend stays bounded so the PLM watchdog cannot fire. - Delete dead CurlMulti::PerformUntilDrained and ActiveRequestCount along with the includes they were the only users of. - Correct the SessionKey comment (security level, not scheme), the request-limit comment, and the header docs so they match the code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d849a6b-2837-4f03-9b04-48dc22e535b4
Andy McCalib (amccalib)
approved these changes
Aug 12, 2026
A non-static data member cannot be declared constexpr, so declaring c_expectedDefaultRequestLimit inside the TAEF test class broke the ADO build. Moved it to namespace scope rather than making it static constexpr, because this project builds as C++14 by default and VERIFY_ARE_EQUAL takes its arguments by const reference, which would odr-use the member and require an out-of-line definition. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d849a6b-2837-4f03-9b04-48dc22e535b4
c_consoleDefaultGlobalRequestLimit is only read inside the GDK branch of DefaultGlobalRequestLimit, so at namespace scope it was an unused const on every other platform. Apple and Linux build with -Werror, which broke the iOS leg. Moved it into the GDK branch where it is used. c_unlimitedGlobalRequestLimit stays at namespace scope because both branches return it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d849a6b-2837-4f03-9b04-48dc22e535b4
Andy McCalib (amccalib)
approved these changes
Aug 12, 2026
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
4 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.
No description provided.