From 789328ad7a9dfac2bb6011165f54b1e5e4c32f14 Mon Sep 17 00:00:00 2001 From: Jason Sandlin Date: Mon, 10 Aug 2026 12:41:59 -0700 Subject: [PATCH 1/6] Add PLM suspend and request-limit features to WinHTTP --- .../libHttpClient.GDK.Shared.vcxitems | 8 - .../libHttpClient.GDK.Shared.vcxitems.filters | 29 +- Build/libHttpClient.GDK.props | 3 +- .../libHttpClient.GDK.NoWebSockets.def | 4 +- Build/libHttpClient.GDK/libHttpClient.GDK.def | 2 + .../libHttpClient.Win32.NoWebSockets.def | 4 +- .../libHttpClient.Win32.def | 2 + Build/libHttpClient.import.props | 7 - Include/httpClient/httpClient.h | 37 +++ Samples/Win32WebSocket/pch.cpp | Bin 430 -> 212 bytes Source/Global/global.cpp | 19 ++ Source/Global/global.h | 8 + Source/Global/global_publics.cpp | 22 ++ Source/HTTP/Curl/CurlDynamicLoader.cpp | 142 --------- Source/HTTP/Curl/CurlDynamicLoader.h | 110 ------- Source/HTTP/Curl/CurlEasyRequest.cpp | 107 +------ Source/HTTP/Curl/CurlEasyRequest.h | 8 +- Source/HTTP/Curl/CurlMulti.cpp | 50 +-- Source/HTTP/Curl/CurlMulti.h | 15 +- Source/HTTP/Curl/CurlProvider.cpp | 156 +-------- Source/HTTP/Curl/CurlProvider.h | 27 +- Source/HTTP/WinHttp/winhttp_connection.cpp | 25 ++ Source/HTTP/WinHttp/winhttp_connection.h | 5 + Source/HTTP/WinHttp/winhttp_provider.cpp | 296 +++++++++++++----- Source/HTTP/WinHttp/winhttp_provider.h | 73 ++++- .../Platform/GDK/PlatformComponents_GDK.cpp | 74 +---- .../Linux/PlatformComponents_Linux.cpp | 8 +- Tests/UnitTests/Tests/AsyncBlockTests.cpp | 17 +- Tests/UnitTests/Tests/GlobalTests.cpp | 107 +++++++ libHttpClient.props | 7 - 30 files changed, 589 insertions(+), 783 deletions(-) delete mode 100644 Source/HTTP/Curl/CurlDynamicLoader.cpp delete mode 100644 Source/HTTP/Curl/CurlDynamicLoader.h diff --git a/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems b/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems index 1d43809f4..aef605484 100644 --- a/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems +++ b/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems @@ -19,10 +19,6 @@ - - - - @@ -49,10 +45,6 @@ - - - - diff --git a/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems.filters b/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems.filters index 4b8f7bbcf..464602cef 100644 --- a/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems.filters +++ b/Build/libHttpClient.GDK.Shared/libHttpClient.GDK.Shared.vcxitems.filters @@ -19,9 +19,6 @@ {cebce833-dc13-4078-9dc3-23dec64d44e6} - - {184db77a-e1d6-458b-ae23-8e7cb1ac3238} - {70210c99-2f90-422b-b92b-53521fc41574} @@ -39,15 +36,6 @@ Source\Common\Win - - Source\HTTP\Curl - - - Source\HTTP\Curl - - - Source\HTTP\Curl - Source\Platform\GDK @@ -60,9 +48,6 @@ Source\Platform\Windows - - Source\HTTP\Curl - Source\WebSocket\Websocketpp @@ -72,24 +57,12 @@ Source\Common\Win - - Source\HTTP\Curl - - - Source\HTTP\Curl - - - Source\HTTP\Curl - Source\WebSocket\WinHttp Source\WebSocket\WinHttp - - Source\HTTP\Curl - Source\WebSocket\Websocketpp @@ -97,4 +70,4 @@ Source\WebSocket\Websocketpp - + \ No newline at end of file diff --git a/Build/libHttpClient.GDK.props b/Build/libHttpClient.GDK.props index 7ef22c71a..880b431e0 100644 --- a/Build/libHttpClient.GDK.props +++ b/Build/libHttpClient.GDK.props @@ -112,7 +112,6 @@ websocketpp; we don't use its serial-port transport, so disable it. --> __WRL_NO_DEFAULT_LIB__;_LIB;ASIO_DISABLE_SERIAL_PORT;$(libHttpClientDefine);%(PreprocessorDefinitions) - %(AdditionalIncludeDirectories);$(GDKCrossPlatformPath)GRDK\ExtensionLibraries\Xbox.XCurl.API\Include Guard /Zc:__cplusplus /ZH:SHA_256 /bigobj /Zi %(AdditionalOptions) HC_PLATFORM=HC_PLATFORM_GDK;HC_DATAMODEL=HC_DATAMODEL_LLP64;%(PreprocessorDefinitions) @@ -124,7 +123,7 @@ false true $(Console_Libs);%(AdditionalDependencies) - xgameruntime.lib;XCurl.lib;%(AdditionalDependencies) + xgameruntime.lib;%(AdditionalDependencies) xgameruntime.lib;%(AdditionalDependencies) diff --git a/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def b/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def index ba63f8d57..115377aba 100644 --- a/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def +++ b/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def @@ -84,6 +84,8 @@ EXPORTS HCMockSetMockMatchedCallback HCRemoveCallRoutedHandler HCSetGlobalProxy + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSettingsGetTraceLevel HCSettingsSetTraceLevel @@ -97,4 +99,4 @@ EXPORTS HCTraceSetPlatformCallbacks HCTraceSetTraceToDebugger HCWinHttpResume - HCWinHttpSuspend \ No newline at end of file + HCWinHttpSuspend diff --git a/Build/libHttpClient.GDK/libHttpClient.GDK.def b/Build/libHttpClient.GDK/libHttpClient.GDK.def index 086ff7c1a..25565f4c9 100644 --- a/Build/libHttpClient.GDK/libHttpClient.GDK.def +++ b/Build/libHttpClient.GDK/libHttpClient.GDK.def @@ -85,6 +85,8 @@ EXPORTS HCRemoveCallRoutedHandler HCRemoveWebSocketRoutedHandler HCSetGlobalProxy + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSetWebSocketFunctions HCSettingsGetTraceLevel diff --git a/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def b/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def index 3b3232907..bff95d8c0 100644 --- a/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def +++ b/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def @@ -79,6 +79,8 @@ EXPORTS HCMockSetMockMatchedCallback HCRemoveCallRoutedHandler HCSetGlobalProxy + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSettingsGetTraceLevel HCSettingsSetTraceLevel @@ -117,4 +119,4 @@ EXPORTS HCHttpCallResponseSetGzipCompressed HCHttpCallRequestSetProgressReportFunction HCHttpCallRequestGetMaxReceiveBufferSize - HCHttpCallRequestSetMaxReceiveBufferSize \ No newline at end of file + HCHttpCallRequestSetMaxReceiveBufferSize diff --git a/Build/libHttpClient.Win32/libHttpClient.Win32.def b/Build/libHttpClient.Win32/libHttpClient.Win32.def index 46f16bc52..16db9173c 100644 --- a/Build/libHttpClient.Win32/libHttpClient.Win32.def +++ b/Build/libHttpClient.Win32/libHttpClient.Win32.def @@ -84,6 +84,8 @@ EXPORTS HCRemoveCallRoutedHandler HCRemoveWebSocketRoutedHandler HCSetGlobalProxy + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSetWebSocketFunctions HCSettingsGetTraceLevel diff --git a/Build/libHttpClient.import.props b/Build/libHttpClient.import.props index 53bb14d43..ff1b197be 100644 --- a/Build/libHttpClient.import.props +++ b/Build/libHttpClient.import.props @@ -65,13 +65,6 @@ - - - - - - - %(AdditionalLibraryDirectories);$(Console_SdkLibPath) diff --git a/Include/httpClient/httpClient.h b/Include/httpClient/httpClient.h index 72d696c1d..abe896d9e 100644 --- a/Include/httpClient/httpClient.h +++ b/Include/httpClient/httpClient.h @@ -211,6 +211,43 @@ STDAPI_(void) HCRemoveCallRoutedHandler( /// If it is passed a null proxy, it will reset to default. Does not include proxying web socket traffic. STDAPI HCSetGlobalProxy(_In_z_ const char* proxyUri) noexcept; +/// +/// Sets the maximum number of HTTP requests allowed to be in flight at one time. +/// +/// The maximum number of concurrent HTTP requests. Passing 0 restores the default of 12. +/// Result code for this API operation. Possible values are S_OK, or E_FAIL. +/// +/// Requests submitted beyond this limit are queued and started automatically as earlier requests +/// complete, so HCHttpCallPerformAsync() never fails because of the limit. Callers may create and +/// submit as many HTTP calls as they like; only the number that reach the platform HTTP stack at +/// once is capped. +/// +/// The limit exists to bound the memory held by in-flight requests, which matters most on +/// memory-constrained titles. The default is 12. +/// +/// This may be called before HCInitialize(). Changing the value does not affect requests that are +/// already in flight, and lowering it will not cancel them; the count drains naturally as they +/// complete. +/// +/// Platform support: the limit is currently enforced only on GDK (Xbox and PC) and Win32, which +/// use the WinHTTP-based HTTP provider. On UWP, Linux, Android, iOS and macOS this value is stored +/// and returned by HCSettingsGetGlobalRequestLimit() but does not throttle requests, because those +/// platforms use HTTP providers that do not implement admission control. +/// +STDAPI HCSettingsSetGlobalRequestLimit(_In_ uint32_t limit) noexcept; + +/// +/// Gets the maximum number of HTTP requests allowed to be in flight at one time. +/// +/// Passes back the current concurrent HTTP request limit. +/// Result code for this API operation. Possible values are S_OK, E_INVALIDARG, or E_FAIL. +/// +/// This may be called before HCInitialize(). Returns the configured value on every platform, which +/// is not necessarily enforced on every platform - see HCSettingsSetGlobalRequestLimit() for the +/// list of platforms where the limit throttles requests. +/// +STDAPI HCSettingsGetGlobalRequestLimit(_Out_ uint32_t* limit) noexcept; + ///////////////////////////////////////////////////////////////////////////////////////// // Http APIs // diff --git a/Samples/Win32WebSocket/pch.cpp b/Samples/Win32WebSocket/pch.cpp index a38f8a37da858b0e7a4a577210e923c883ac97f3..607a2953f2a953167dfad188d7dc83ad9aefe4c6 100644 GIT binary patch literal 212 zcmZvWF%H5o5CnU_Vg)Ub9$vsR%z2YoggfhO7rc#vC-DUmYRc7UH2ZnIhXFRf6a50Eu-52LjOMGe;n=e2X4Ec1rV#d7W$B67 zZK-(|!dks5CMrEH2jof3i8kl@s~>Rd4R^92H$=Ct33KaLJCW^ literal 430 zcma)(OAdlS5JYQj;vFWsRoUwaJO;ubLXfO} ziM!*RK?b#&)PqF4LX~Q_GVq=g$>^{f<-{7@$r|ykmqm|MwtC_%l{t|TJS9Jpalvn} ztpC7C!Dg_+`-h+El+KlNtsDc|GOvyF+iK(A-j!r1ot!0T2mdlziM{46Ghg8k=#H+V zM?tLvNCnoo3Y&aHb%nm~4lL&e!^r g_globalRequestLimit{ c_defaultGlobalRequestLimit }; + +void SetGlobalRequestLimit(uint32_t limit) noexcept +{ + // A limit of 0 would stall every request forever with no way to recover, so treat it as + // "restore the default" rather than silently wedging the title. + g_globalRequestLimit.store(limit == 0 ? c_defaultGlobalRequestLimit : limit, std::memory_order_relaxed); +} + +uint32_t GetGlobalRequestLimit() noexcept +{ + return g_globalRequestLimit.load(std::memory_order_relaxed); +} + HRESULT http_singleton::singleton_access( _In_ singleton_access_mode mode, _In_opt_ HCInitArgs* createArgs, diff --git a/Source/Global/global.h b/Source/Global/global.h index e35b31ace..b834f08ad 100644 --- a/Source/Global/global.h +++ b/Source/Global/global.h @@ -12,6 +12,14 @@ namespace log class logger; } +// Process-wide cap on the number of HTTP requests allowed in flight against the platform HTTP +// stack at once. Kept outside the http_singleton so it can be configured before HCInitialize. +// Enforcement lives in the platform HTTP provider; only the WinHTTP provider (GDK and Win32) +// implements admission control today, so on other platforms this value is stored and readable but +// does not throttle. +void SetGlobalRequestLimit(uint32_t limit) noexcept; +uint32_t GetGlobalRequestLimit() noexcept; + typedef struct http_retry_after_api_state { http_retry_after_api_state() = default; diff --git a/Source/Global/global_publics.cpp b/Source/Global/global_publics.cpp index 32e7a450e..2629b82ab 100644 --- a/Source/Global/global_publics.cpp +++ b/Source/Global/global_publics.cpp @@ -81,6 +81,28 @@ try } CATCH_RETURN() +STDAPI +HCSettingsSetGlobalRequestLimit(_In_ uint32_t limit) noexcept +try +{ + // Deliberately does not require initialization: the limit is process-wide state so it can be + // configured before HCInitialize creates the provider. + xbox::httpclient::SetGlobalRequestLimit(limit); + return S_OK; +} +CATCH_RETURN() + +STDAPI +HCSettingsGetGlobalRequestLimit(_Out_ uint32_t* limit) noexcept +try +{ + RETURN_HR_IF(E_INVALIDARG, !limit); + + *limit = xbox::httpclient::GetGlobalRequestLimit(); + return S_OK; +} +CATCH_RETURN() + STDAPI HCSetHttpCallPerformFunction( _In_ HCCallPerformFunction performFunc, diff --git a/Source/HTTP/Curl/CurlDynamicLoader.cpp b/Source/HTTP/Curl/CurlDynamicLoader.cpp deleted file mode 100644 index 1621e8eda..000000000 --- a/Source/HTTP/Curl/CurlDynamicLoader.cpp +++ /dev/null @@ -1,142 +0,0 @@ -#include "pch.h" -#include "CurlDynamicLoader.h" - -#if HC_PLATFORM == HC_PLATFORM_GDK - -#include -#include - -namespace xbox -{ -namespace httpclient -{ - -std::mutex CurlDynamicLoader::s_initMutex; -HC_UNIQUE_PTR CurlDynamicLoader::s_instance; - -CurlDynamicLoader& CurlDynamicLoader::GetInstance() -{ - std::lock_guard lock(s_initMutex); - if (!s_instance) - { - HC_TRACE_VERBOSE(HTTPCLIENT, "Creating CurlDynamicLoader instance"); - - // Use libHttpClient custom allocator hooks while staying within class access to private ctor - http_stl_allocator a{}; - s_instance = HC_UNIQUE_PTR{ new (a.allocate(1)) CurlDynamicLoader }; - } - return *s_instance; -} - -void CurlDynamicLoader::DestroyInstance() -{ - std::lock_guard lock(s_initMutex); - if (s_instance) - { - // Unique ptr with http_alloc_deleter ensures custom free hooks are used - s_instance.reset(); - } -} - -CurlDynamicLoader::~CurlDynamicLoader() -{ - Cleanup(); -} - -bool CurlDynamicLoader::Initialize() -{ - if (m_curlLibrary != nullptr) - { - HC_TRACE_VERBOSE(HTTPCLIENT, "XCurl.dll already loaded"); - return true; // Already loaded - } - - HC_TRACE_INFORMATION(HTTPCLIENT, "Attempting to load XCurl.dll"); - - // Try to load XCurl.dll - m_curlLibrary = LoadLibraryA("XCurl.dll"); - if (m_curlLibrary == nullptr) - { - DWORD error = GetLastError(); - HC_TRACE_ERROR(HTTPCLIENT, "Failed to load XCurl.dll. Error code: %lu", error); - return false; - } - - // Load all required functions - bool success = true; - - success &= LoadFunction(reinterpret_cast(curl_global_init_fn), "curl_global_init"); - success &= LoadFunction(reinterpret_cast(curl_global_cleanup_fn), "curl_global_cleanup"); - success &= LoadFunction(reinterpret_cast(curl_easy_init_fn), "curl_easy_init"); - success &= LoadFunction(reinterpret_cast(curl_easy_cleanup_fn), "curl_easy_cleanup"); - success &= LoadFunction(reinterpret_cast(curl_easy_setopt_fn), "curl_easy_setopt"); - success &= LoadFunction(reinterpret_cast(curl_easy_getinfo_fn), "curl_easy_getinfo"); - success &= LoadFunction(reinterpret_cast(curl_easy_strerror_fn), "curl_easy_strerror"); - success &= LoadFunction(reinterpret_cast(curl_slist_append_fn), "curl_slist_append"); - success &= LoadFunction(reinterpret_cast(curl_slist_free_all_fn), "curl_slist_free_all"); - success &= LoadFunction(reinterpret_cast(curl_multi_init_fn), "curl_multi_init"); - success &= LoadFunction(reinterpret_cast(curl_multi_cleanup_fn), "curl_multi_cleanup"); - success &= LoadFunction(reinterpret_cast(curl_multi_add_handle_fn), "curl_multi_add_handle"); - success &= LoadFunction(reinterpret_cast(curl_multi_remove_handle_fn), "curl_multi_remove_handle"); - success &= LoadFunction(reinterpret_cast(curl_multi_perform_fn), "curl_multi_perform"); - success &= LoadFunction(reinterpret_cast(curl_multi_info_read_fn), "curl_multi_info_read"); - - // Note: curl_multi_poll might not be available in older versions, so we make it optional - LoadFunction(reinterpret_cast(curl_multi_poll_fn), "curl_multi_poll"); - success &= LoadFunction(reinterpret_cast(curl_multi_wait_fn), "curl_multi_wait"); - - if (!success) - { - Cleanup(); - return false; - } - - HC_TRACE_INFORMATION(HTTPCLIENT, "XCurl.dll loaded successfully"); - return true; -} - -void CurlDynamicLoader::Cleanup() -{ - if (m_curlLibrary != nullptr) - { - HC_TRACE_INFORMATION(HTTPCLIENT, "Unloading XCurl.dll"); - FreeLibrary(m_curlLibrary); - m_curlLibrary = nullptr; - } - - // Reset all function pointers - curl_global_init_fn = nullptr; - curl_global_cleanup_fn = nullptr; - curl_easy_init_fn = nullptr; - curl_easy_cleanup_fn = nullptr; - curl_easy_setopt_fn = nullptr; - curl_easy_getinfo_fn = nullptr; - curl_easy_strerror_fn = nullptr; - curl_slist_append_fn = nullptr; - curl_slist_free_all_fn = nullptr; - curl_multi_init_fn = nullptr; - curl_multi_cleanup_fn = nullptr; - curl_multi_add_handle_fn = nullptr; - curl_multi_remove_handle_fn = nullptr; - curl_multi_perform_fn = nullptr; - curl_multi_info_read_fn = nullptr; - curl_multi_poll_fn = nullptr; - curl_multi_wait_fn = nullptr; -} - -bool CurlDynamicLoader::LoadFunction(FARPROC& funcPtr, const char* functionName) -{ - funcPtr = GetProcAddress(m_curlLibrary, functionName); - if (funcPtr == nullptr) - { - DWORD error = GetLastError(); - HC_TRACE_ERROR(HTTPCLIENT, "Failed to load function: %s. Error code: %lu", functionName, error); - return false; - } - return true; -} - -} // httpclient -} // xbox - -#endif // HC_PLATFORM == HC_PLATFORM_GDK diff --git a/Source/HTTP/Curl/CurlDynamicLoader.h b/Source/HTTP/Curl/CurlDynamicLoader.h deleted file mode 100644 index 821cede41..000000000 --- a/Source/HTTP/Curl/CurlDynamicLoader.h +++ /dev/null @@ -1,110 +0,0 @@ -#pragma once - -// -// This header is always includable across platforms. On non-GDK platforms, -// the macros are defined as direct calls and the dynamic loader class is absent. -// On GDK, the dynamic loader class is available and macros route through it. -// - -#if HC_PLATFORM == HC_PLATFORM_GDK - -#include -#include -#include -#include - -namespace xbox -{ -namespace httpclient -{ - -// Dynamic curl function pointers -class CurlDynamicLoader -{ -public: - // Initialization/Cleanup functions - using curl_global_init_ptr = CURLcode(*)(long flags); - using curl_global_cleanup_ptr = void(*)(); - - // Easy interface functions - using curl_easy_init_ptr = CURL*(*)(); - using curl_easy_cleanup_ptr = void(*)(CURL* curl); - using curl_easy_setopt_ptr = CURLcode(*)(CURL* curl, CURLoption option, ...); - using curl_easy_getinfo_ptr = CURLcode(*)(CURL* curl, CURLINFO info, ...); - using curl_easy_strerror_ptr = const char*(*)(CURLcode code); - - // String list functions - using curl_slist_append_ptr = struct curl_slist*(*)(struct curl_slist* list, const char* string); - using curl_slist_free_all_ptr = void(*)(struct curl_slist* list); - - // Multi interface functions - using curl_multi_init_ptr = CURLM*(*)(); - using curl_multi_cleanup_ptr = CURLMcode(*)(CURLM* multi_handle); - using curl_multi_add_handle_ptr = CURLMcode(*)(CURLM* multi_handle, CURL* curl_handle); - using curl_multi_remove_handle_ptr = CURLMcode(*)(CURLM* multi_handle, CURL* curl_handle); - using curl_multi_perform_ptr = CURLMcode(*)(CURLM* multi_handle, int* running_handles); - using curl_multi_info_read_ptr = CURLMsg*(*)(CURLM* multi_handle, int* msgs_in_queue); - using curl_multi_poll_ptr = CURLMcode(*)(CURLM* multi_handle, struct curl_waitfd extra_fds[], unsigned int extra_nfds, int timeout_ms, int* ret); - using curl_multi_wait_ptr = CURLMcode(*)(CURLM* multi_handle, struct curl_waitfd extra_fds[], unsigned int extra_nfds, int timeout_ms, int* numfds); - - // Function pointers - curl_global_init_ptr curl_global_init_fn = nullptr; - curl_global_cleanup_ptr curl_global_cleanup_fn = nullptr; - curl_easy_init_ptr curl_easy_init_fn = nullptr; - curl_easy_cleanup_ptr curl_easy_cleanup_fn = nullptr; - curl_easy_setopt_ptr curl_easy_setopt_fn = nullptr; - curl_easy_getinfo_ptr curl_easy_getinfo_fn = nullptr; - curl_easy_strerror_ptr curl_easy_strerror_fn = nullptr; - curl_slist_append_ptr curl_slist_append_fn = nullptr; - curl_slist_free_all_ptr curl_slist_free_all_fn = nullptr; - curl_multi_init_ptr curl_multi_init_fn = nullptr; - curl_multi_cleanup_ptr curl_multi_cleanup_fn = nullptr; - curl_multi_add_handle_ptr curl_multi_add_handle_fn = nullptr; - curl_multi_remove_handle_ptr curl_multi_remove_handle_fn = nullptr; - curl_multi_perform_ptr curl_multi_perform_fn = nullptr; - curl_multi_info_read_ptr curl_multi_info_read_fn = nullptr; - curl_multi_poll_ptr curl_multi_poll_fn = nullptr; - curl_multi_wait_ptr curl_multi_wait_fn = nullptr; - - static CurlDynamicLoader& GetInstance(); - // Frees the singleton instance and unloads XCurl.dll (via destructor -> Cleanup) - static void DestroyInstance(); - ~CurlDynamicLoader(); - - bool Initialize(); - void Cleanup(); - bool IsLoaded() const { return m_curlLibrary != nullptr; } - -private: - CurlDynamicLoader() = default; - - bool LoadFunction(FARPROC& funcPtr, const char* functionName); - - HMODULE m_curlLibrary = nullptr; - - // Thread safety - static std::mutex s_initMutex; - static HC_UNIQUE_PTR s_instance; -}; - -} // httpclient -} // xbox - -// GDK macro variants: route through dynamic loader and provide default returns when not loaded -#define CURL_CALL(func_name) ::xbox::httpclient::CurlDynamicLoader::GetInstance().func_name##_fn -#define CURL_INVOKE_OR(defaultRet, func, ...) \ - ((::xbox::httpclient::CurlDynamicLoader::GetInstance().IsLoaded()) ? \ - (::xbox::httpclient::CurlDynamicLoader::GetInstance().func##_fn(__VA_ARGS__)) : \ - (defaultRet)) -// Convenience when defaultRet == 0 (common for void-calls or zero-initialized return types) -#define CURL_INVOKE(func, ...) CURL_INVOKE_OR(0, func, __VA_ARGS__) - -#else // non-GDK - -// Non-GDK macro variants: call directly -#define CURL_CALL(func_name) func_name -#define CURL_INVOKE_OR(defaultRet, func, ...) func(__VA_ARGS__) -// Convenience when defaultRet == 0 -#define CURL_INVOKE(func, ...) func(__VA_ARGS__) - -#endif // HC_PLATFORM == HC_PLATFORM_GDK diff --git a/Source/HTTP/Curl/CurlEasyRequest.cpp b/Source/HTTP/Curl/CurlEasyRequest.cpp index ce8eec075..21e45ec94 100644 --- a/Source/HTTP/Curl/CurlEasyRequest.cpp +++ b/Source/HTTP/Curl/CurlEasyRequest.cpp @@ -18,21 +18,13 @@ CurlEasyRequest::CurlEasyRequest(CURL* curlEasyHandle, HCCallHandle hcCall, XAsy CurlEasyRequest::~CurlEasyRequest() { - (void)CURL_INVOKE(curl_easy_cleanup, m_curlEasyHandle); - (void)CURL_INVOKE(curl_slist_free_all, m_headers); + (void)curl_easy_cleanup(m_curlEasyHandle); + (void)curl_slist_free_all(m_headers); } Result> CurlEasyRequest::Initialize(HCCallHandle hcCall, XAsyncBlock* async) { -#if HC_PLATFORM == HC_PLATFORM_GDK - // Ensure curl is loaded - if (!CurlDynamicLoader::GetInstance().IsLoaded()) - { - HC_TRACE_ERROR(HTTPCLIENT, "CurlEasyRequest::Initialize: XCurl.dll not available"); - return E_HC_XCURL_REQUIRED; - } -#endif - CURL* curlEasyHandle{ CURL_CALL(curl_easy_init)() }; + CURL* curlEasyHandle{ curl_easy_init() }; if (!curlEasyHandle) { HC_TRACE_ERROR(HTTPCLIENT, "CurlEasyRequest::Initialize:: curl_easy_init failed"); @@ -48,8 +40,7 @@ Result> CurlEasyRequest::Initialize(HCCallHandle void* clientRequestBodyReadCallbackContext{}; RETURN_IF_FAILED(HCHttpCallRequestGetRequestBodyReadFunction(hcCall, &clientRequestBodyReadCallback, &bodySize, &clientRequestBodyReadCallbackContext)); -// Specify libcurl progress callback and create libcurl progress callback for non-GDK platforms since XCurl doesn't support libcurl progress callback -#if HC_PLATFORM != HC_PLATFORM_GDK +// Specify libcurl's progress callback if the caller asked for progress reporting. // Get LHC Progress callback functions size_t uploadMinimumProgressInterval; void* uploadProgressReportCallbackContext{}; @@ -68,32 +59,18 @@ Result> CurlEasyRequest::Initialize(HCCallHandle easyRequest->SetOpt(CURLOPT_XFERINFOFUNCTION, &ProgressReportCallback); easyRequest->SetOpt(CURLOPT_NOPROGRESS, 0L); } -#endif // we set both POSTFIELDSIZE and INFILESIZE because curl uses one or the // other depending on method // We are allowing Setops to happen with a bodySize of zero in linux to handle certain clients // not being able to handle handshakes without a fixed body size. - // The reason for an if def statement is to handle the behavioral differences in libCurl vs xCurl. - -#if HC_PLATFORM == HC_PLATFORM_GDK - if (bodySize > 0) - { - RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_POSTFIELDSIZE, static_cast(bodySize))); - RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_INFILESIZE, static_cast(bodySize))); - // read callback - RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_READFUNCTION, &ReadCallback)); - RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_READDATA, easyRequest.get())); - } -#else RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_POSTFIELDSIZE, static_cast(bodySize))); RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_INFILESIZE, static_cast(bodySize))); // read callback RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_READFUNCTION, &ReadCallback)); RETURN_IF_FAILED(easyRequest->SetOpt(CURLOPT_READDATA, easyRequest.get())); -#endif // url & method char const* url = nullptr; @@ -183,7 +160,7 @@ void CurlEasyRequest::Complete(CURLcode result) HC_TRACE_INFORMATION(HTTPCLIENT, "CurlEasyRequest::m_errorBuffer='%s'", m_errorBuffer); long platformError = 0; - auto curle = CURL_CALL(curl_easy_getinfo)(m_curlEasyHandle, CURLINFO_OS_ERRNO, &platformError); + auto curle = curl_easy_getinfo(m_curlEasyHandle, CURLINFO_OS_ERRNO, &platformError); if (curle != CURLE_OK) { return Fail(HrFromCurle(curle)); @@ -192,13 +169,13 @@ void CurlEasyRequest::Complete(CURLcode result) HRESULT hr = HCHttpCallResponseSetNetworkErrorCode(m_hcCallHandle, E_FAIL, static_cast(platformError)); assert(SUCCEEDED(hr)); - hr = HCHttpCallResponseSetPlatformNetworkErrorMessage(m_hcCallHandle, CURL_CALL(curl_easy_strerror)(result)); + hr = HCHttpCallResponseSetPlatformNetworkErrorMessage(m_hcCallHandle, curl_easy_strerror(result)); assert(SUCCEEDED(hr)); } else { long httpStatus = 0; - auto curle = CURL_CALL(curl_easy_getinfo)(m_curlEasyHandle, CURLINFO_RESPONSE_CODE, &httpStatus); + auto curle = curl_easy_getinfo(m_curlEasyHandle, CURLINFO_RESPONSE_CODE, &httpStatus); if (curle != CURLE_OK) { return Fail(HrFromCurle(curle)); @@ -240,7 +217,7 @@ HRESULT CurlEasyRequest::AddHeader(char const* name, char const* value) noexcept } header.resize(static_cast(written)); - curl_slist* appended = CURL_CALL(curl_slist_append)(m_headers, header.c_str()); + curl_slist* appended = curl_slist_append(m_headers, header.c_str()); if (!appended) { m_headersBuffer.pop_back(); @@ -286,39 +263,6 @@ size_t CurlEasyRequest::ReadCallback(char* buffer, size_t size, size_t nitems, v request->m_requestBodyOffset += bytesWritten; -#if HC_PLATFORM == HC_PLATFORM_GDK - size_t uploadMinimumProgressInterval; - void* uploadProgressReportCallbackContext{}; - HCHttpCallProgressReportFunction uploadProgressReportFunction = nullptr; - hr = HCHttpCallRequestGetProgressReportFunction(request->m_hcCallHandle, true, &uploadProgressReportFunction, &uploadMinimumProgressInterval, &uploadProgressReportCallbackContext); - if (FAILED(hr)) - { - HC_TRACE_ERROR_HR(HTTPCLIENT, hr, "CurlEasyRequest::ReadCallback: failed getting Progress Report upload function"); - return 1; - } - - uint64_t dynamicBodySize{}; - uint64_t dynamicBodyBytesWritten{}; - HCHttpCallRequestGetDynamicBytesWritten(request->m_hcCallHandle, &dynamicBodySize, &dynamicBodyBytesWritten); - - uint64_t reportBytesWritten = request->m_requestBodyOffset; - uint64_t reportTotalBytes = bodySize; - if (dynamicBodySize > 0) - { - reportBytesWritten = dynamicBodyBytesWritten; - reportTotalBytes = dynamicBodySize; - } - - ReportProgress( - request->m_hcCallHandle, - uploadProgressReportFunction, - request->m_hcCallHandle->uploadMinimumProgressReportInterval, - reportBytesWritten, - reportTotalBytes, - uploadProgressReportCallbackContext, - &request->m_hcCallHandle->uploadLastProgressReport - ); -#endif return bytesWritten; } @@ -390,7 +334,7 @@ size_t CurlEasyRequest::WriteHeaderCallback(char* buffer, size_t size, size_t ni size_t CurlEasyRequest::GetResponseContentLength(CURL* curlHandle) { curl_off_t contentLength = 0; - CURL_CALL(curl_easy_getinfo)(curlHandle, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T, &contentLength); + curl_easy_getinfo(curlHandle, CURLINFO_CONTENT_LENGTH_DOWNLOAD_T, &contentLength); return contentLength; } @@ -436,39 +380,6 @@ size_t CurlEasyRequest::WriteDataCallback(char* buffer, size_t size, size_t nmem { request->m_responseBodyRemainingToRead -= bufferSize; -#if HC_PLATFORM == HC_PLATFORM_GDK - size_t downloadMinimumProgressInterval; - void* downloadProgressReportCallbackContext{}; - HCHttpCallProgressReportFunction downloadProgressReportFunction = nullptr; - hr = HCHttpCallRequestGetProgressReportFunction(request->m_hcCallHandle, false, &downloadProgressReportFunction, &downloadMinimumProgressInterval, &downloadProgressReportCallbackContext); - if (FAILED(hr)) - { - HC_TRACE_ERROR_HR(HTTPCLIENT, hr, "CurlEasyRequest::WriteDataCallback: failed getting Progress Report download function"); - return 1; - } - - uint64_t dynamicBodySize{}; - uint64_t dynamicBodyBytesWritten{}; - HCHttpCallResponseGetDynamicBytesWritten(request->m_hcCallHandle, &dynamicBodySize, &dynamicBodyBytesWritten); - - uint64_t reportBytesWritten = request->m_responseBodySize - request->m_responseBodyRemainingToRead; - uint64_t reportTotalBytes = request->m_responseBodySize; - if (dynamicBodySize > 0) - { - reportBytesWritten = dynamicBodyBytesWritten; - reportTotalBytes = dynamicBodySize; - } - - ReportProgress( - request->m_hcCallHandle, - downloadProgressReportFunction, - request->m_hcCallHandle->downloadMinimumProgressReportInterval, - reportBytesWritten, - reportTotalBytes, - downloadProgressReportCallbackContext, - &request->m_hcCallHandle->downloadLastProgressReport - ); -#endif } return bufferSize; diff --git a/Source/HTTP/Curl/CurlEasyRequest.h b/Source/HTTP/Curl/CurlEasyRequest.h index 1c1e1cb87..916ad8ee9 100644 --- a/Source/HTTP/Curl/CurlEasyRequest.h +++ b/Source/HTTP/Curl/CurlEasyRequest.h @@ -1,11 +1,5 @@ #pragma once -// Always include CurlDynamicLoader.h for macros and (on GDK) loader type -#include "CurlDynamicLoader.h" -// Http provider should work with other curl implementations as well. -// The logic in CurlMulti::Perform is optimized for XCurl, but should work on any curl implementation. -#if HC_PLATFORM != HC_PLATFORM_GDK #include -#endif #include "Result.h" namespace xbox @@ -76,7 +70,7 @@ class CurlEasyRequest template HRESULT CurlEasyRequest::SetOpt(CURLoption option, typename OptType::type v) noexcept { - CURLcode result = CURL_CALL(curl_easy_setopt)(m_curlEasyHandle, option, v); + CURLcode result = curl_easy_setopt(m_curlEasyHandle, option, v); if (result != CURLE_OK) { HC_TRACE_ERROR(HTTPCLIENT, "curl_easy_setopt(request, %d, value) failed with %d", option, result); diff --git a/Source/HTTP/Curl/CurlMulti.cpp b/Source/HTTP/Curl/CurlMulti.cpp index 8715e8e8a..35ec6e678 100644 --- a/Source/HTTP/Curl/CurlMulti.cpp +++ b/Source/HTTP/Curl/CurlMulti.cpp @@ -1,6 +1,5 @@ #include "pch.h" #include "CurlMulti.h" -#include "CurlDynamicLoader.h" #include "CurlProvider.h" #include @@ -11,7 +10,7 @@ namespace xbox namespace httpclient { -// XCurl doesn't support curl_multi_timeout, so use a small, fixed delay between calls to curl_multi_perform +// Some curl implementations do not support curl_multi_timeout, so use a small, fixed delay between calls to curl_multi_perform #define PERFORM_DELAY_MS 50 #define POLL_TIMEOUT_MS 0 @@ -19,19 +18,10 @@ Result> CurlMulti::Initialize(XTaskQueuePortHandle work { assert(workPort); -#if HC_PLATFORM == HC_PLATFORM_GDK - // Ensure curl is loaded - if (!CurlDynamicLoader::GetInstance().IsLoaded()) - { - HC_TRACE_ERROR(HTTPCLIENT, "CurlMulti::Initialize: XCurl.dll not available"); - return E_HC_XCURL_REQUIRED; - } -#endif - http_stl_allocator a{}; HC_UNIQUE_PTR multi{ new (a.allocate(1)) CurlMulti }; - multi->m_curlMultiHandle = CURL_CALL(curl_multi_init)(); + multi->m_curlMultiHandle = curl_multi_init(); if (!multi->m_curlMultiHandle) { HC_TRACE_ERROR(HTTPCLIENT, "CurlMulti::Initialize: curl_multi_init failed"); @@ -52,13 +42,13 @@ CurlMulti::~CurlMulti() if (!m_easyRequests.empty()) { - HC_TRACE_WARNING(HTTPCLIENT, "CurlMulti::~XCurlMulti: Failing all active requests."); + HC_TRACE_WARNING(HTTPCLIENT, "CurlMulti::~CurlMulti: Failing all active requests."); FailAllRequests(E_UNEXPECTED); } if (m_curlMultiHandle) { - (void)CURL_INVOKE(curl_multi_cleanup, m_curlMultiHandle); + (void)curl_multi_cleanup(m_curlMultiHandle); } } @@ -72,7 +62,7 @@ HRESULT CurlMulti::AddRequest(HC_UNIQUE_PTR easyRequest) return E_FAIL; } - auto result = CURL_CALL(curl_multi_add_handle)(m_curlMultiHandle, easyRequest->Handle()); + auto result = curl_multi_add_handle(m_curlMultiHandle, easyRequest->Handle()); if (result != CURLM_OK) { HC_TRACE_ERROR(HTTPCLIENT, "CurlMulti::AddRequest: curl_multi_add_handle failed with CURLCode=%u", result); @@ -212,7 +202,7 @@ void CALLBACK CurlMulti::TaskQueueCallback(_In_opt_ void* context, _In_ bool can HRESULT CurlMulti::PerformStepLocked(int& runningRequests) noexcept { runningRequests = 0; - CURLMcode result = CURL_CALL(curl_multi_perform)(m_curlMultiHandle, &runningRequests); + CURLMcode result = curl_multi_perform(m_curlMultiHandle, &runningRequests); if (result != CURLM_OK) { HC_TRACE_ERROR(HTTPCLIENT, "CurlMulti::PerformStepLocked: curl_multi_perform failed with CURLMcode=%u", result); @@ -222,7 +212,7 @@ HRESULT CurlMulti::PerformStepLocked(int& runningRequests) noexcept int remainingMessages{ 1 }; // assume there is at least 1 message so loop is always entered while (remainingMessages) { - CURLMsg* message = CURL_CALL(curl_multi_info_read)(m_curlMultiHandle, &remainingMessages); + CURLMsg* message = curl_multi_info_read(m_curlMultiHandle, &remainingMessages); if (message) { switch (message->msg) @@ -232,7 +222,7 @@ HRESULT CurlMulti::PerformStepLocked(int& runningRequests) noexcept auto requestIter = m_easyRequests.find(message->easy_handle); assert(requestIter != m_easyRequests.end()); - result = CURL_CALL(curl_multi_remove_handle)(m_curlMultiHandle, message->easy_handle); + result = curl_multi_remove_handle(m_curlMultiHandle, message->easy_handle); if (result != CURLM_OK) { HC_TRACE_ERROR(HTTPCLIENT, "CurlMulti::PerformStepLocked: curl_multi_remove_handle failed with CURLMcode=%u", result); @@ -269,24 +259,12 @@ HRESULT CurlMulti::Perform() noexcept // Reschedule Perform if there are still running requests int workAvailable{ 0 }; CURLMcode result{ CURLM_OK }; -#if HC_PLATFORM == HC_PLATFORM_GDK - // Try curl_multi_poll first, fall back to curl_multi_wait if not available - if (CURL_CALL(curl_multi_poll)) - { - result = CURL_CALL(curl_multi_poll)(m_curlMultiHandle, nullptr, 0, POLL_TIMEOUT_MS, &workAvailable); - } - else - { - result = CURL_CALL(curl_multi_wait)(m_curlMultiHandle, nullptr, 0, POLL_TIMEOUT_MS, &workAvailable); - } -#elif defined(CURL_AT_LEAST_VERSION) && CURL_AT_LEAST_VERSION(7,69,0) - // On supported non-GDK platforms with libcurl >= 7.69.0, we can call curl_multi_poll directly. - static_assert(CURL_CALL(curl_multi_poll) == curl_multi_poll, "curl_multi_poll must be unconditionally available"); - result = CURL_CALL(curl_multi_poll)(m_curlMultiHandle, nullptr, 0, POLL_TIMEOUT_MS, &workAvailable); +#if defined(CURL_AT_LEAST_VERSION) && CURL_AT_LEAST_VERSION(7,69,0) + // With libcurl >= 7.69.0 we can call curl_multi_poll directly. + result = curl_multi_poll(m_curlMultiHandle, nullptr, 0, POLL_TIMEOUT_MS, &workAvailable); #else - // On supported non-GDK platforms with libcurl < 7.69.0, we must fall back to curl_multi_wait. - static_assert(CURL_CALL(curl_multi_wait) == curl_multi_wait, "curl_multi_wait must be unconditionally available"); - result = CURL_CALL(curl_multi_wait)(m_curlMultiHandle, nullptr, 0, POLL_TIMEOUT_MS, &workAvailable); + // With libcurl < 7.69.0 we must fall back to curl_multi_wait. + result = curl_multi_wait(m_curlMultiHandle, nullptr, 0, POLL_TIMEOUT_MS, &workAvailable); #endif UNREFERENCED_PARAMETER(result); @@ -360,7 +338,7 @@ void CurlMulti::FailAllRequests(HRESULT hr) noexcept { for (auto& pair : m_easyRequests) { - auto result = CURL_INVOKE_OR(CURLM_OK, curl_multi_remove_handle, m_curlMultiHandle, pair.first); + auto result = curl_multi_remove_handle(m_curlMultiHandle, pair.first); if (FAILED(HrFromCurlm(result))) { HC_TRACE_ERROR(HTTPCLIENT, "CurlMulti::FailAllRequests: curl_multi_remove_handle failed with CURLCode=%u", result); diff --git a/Source/HTTP/Curl/CurlMulti.h b/Source/HTTP/Curl/CurlMulti.h index b727fc307..eec42b64c 100644 --- a/Source/HTTP/Curl/CurlMulti.h +++ b/Source/HTTP/Curl/CurlMulti.h @@ -24,14 +24,17 @@ class CurlMulti // completed or timeoutMs elapses. // // The normal perform loop only advances when the caller-supplied task queue is being - // dispatched (see ScheduleTaskQueueCallback). During an app suspend a title is free to - // park its own queue, which would otherwise stall the loop and leave xCurl blocked in - // Curl_multi::WaitForActiveHandles until the suspend watchdog terminates the title - // (bug 63050439). The xCurl contract puts the "keep performing" duty on the multi - // consumer, so on suspend LHC drives the loop itself instead of relying on that queue. + // dispatched (see ScheduleTaskQueueCallback), so this exists for callers that must make + // progress without that queue - originally the GDK suspend path for bug 63050439. + // + // NOTE: currently unreferenced. Its only caller was CurlProvider's GDK suspend handling, + // which was removed when GDK moved to the WinHTTP provider. Kept because it is provider + // -agnostic and the same need arises for any caller that must drain without the queue; + // delete it if no such caller materializes. HRESULT PerformUntilDrained(uint32_t timeoutMs) noexcept; - // Number of requests still owned by this multi handle. + // Number of requests still owned by this multi handle. Currently only used by + // PerformUntilDrained. size_t ActiveRequestCount() noexcept; // Asyncronously cleanup any outstanding requests diff --git a/Source/HTTP/Curl/CurlProvider.cpp b/Source/HTTP/Curl/CurlProvider.cpp index 87abb6040..09beaf907 100644 --- a/Source/HTTP/Curl/CurlProvider.cpp +++ b/Source/HTTP/Curl/CurlProvider.cpp @@ -1,9 +1,6 @@ #include "pch.h" #include "CurlProvider.h" #include "CurlEasyRequest.h" -#include "CurlDynamicLoader.h" - -#include namespace xbox { @@ -25,9 +22,7 @@ HRESULT HrFromCurlm(CURLMcode c) noexcept switch (c) { case CURLMcode::CURLM_OK: return S_OK; -#if HC_PLATFORM == HC_PLATFORM_GDK - case CURLMcode::CURLM_BAD_FUNCTION_ARGUMENT: assert(false); return E_INVALIDARG; -#elif defined(CURL_AT_LEAST_VERSION) && CURL_AT_LEAST_VERSION(7,69,0) +#if defined(CURL_AT_LEAST_VERSION) && CURL_AT_LEAST_VERSION(7,69,0) case CURLMcode::CURLM_BAD_FUNCTION_ARGUMENT: assert(false); return E_INVALIDARG; #endif default: return E_FAIL; @@ -36,48 +31,12 @@ HRESULT HrFromCurlm(CURLMcode c) noexcept Result> CurlProvider::Initialize() { -#if HC_PLATFORM == HC_PLATFORM_GDK - // Initialize dynamic curl loader first - auto& loader = CurlDynamicLoader::GetInstance(); - if (!loader.Initialize()) - { - HC_TRACE_ERROR(HTTPCLIENT, "CurlProvider::Initialize: Failed to load XCurl.dll"); - // Ensure the loader is cleaned up if initialization fails - CurlDynamicLoader::DestroyInstance(); - return E_FAIL; - } - - CURLcode initRes = CURL_CALL(curl_global_init)(CURL_GLOBAL_ALL); - HRESULT initHr = HrFromCurle(initRes); - if (FAILED(initHr)) - { - // If curl init fails, unload XCurl and free the loader singleton - CurlDynamicLoader::DestroyInstance(); - return initHr; - } -#else - CURLcode initRes = CURL_CALL(curl_global_init)(CURL_GLOBAL_ALL); + CURLcode initRes = curl_global_init(CURL_GLOBAL_ALL); RETURN_IF_FAILED(HrFromCurle(initRes)); -#endif http_stl_allocator a{}; auto provider = HC_UNIQUE_PTR{ new (a.allocate(1)) CurlProvider }; -#if HC_PLATFORM == HC_PLATFORM_GDK - // Mirror WinHttpProvider: subscribe to PLM app state so the provider can keep the curl - // perform loop running while the title suspends. Without this the loop only advances when - // the title dispatches its own task queue, and a title that parks that queue on suspend - // leaves xCurl blocked in WaitForActiveHandles until the watchdog kills it (bug 63050439). - HRESULT registerHr = RegisterAppStateChangeNotification(CurlProvider::AppStateChangedCallback, provider.get(), &provider->m_appStateChangedToken); - if (FAILED(registerHr)) - { - // Suspend handling is a resilience feature; failing to subscribe must not stop HTTP from - // working, so log and continue rather than failing initialization. - HC_TRACE_ERROR_HR(HTTPCLIENT, registerHr, "CurlProvider::Initialize: RegisterAppStateChangeNotification failed; suspend handling disabled"); - provider->m_appStateChangedToken = nullptr; - } -#endif - return std::move(provider); } @@ -86,45 +45,19 @@ CurlProvider::~CurlProvider() // Either CleanupAsync was never called or CurlProvider shouldn't be destroyed until it completes. assert(!m_cleanupTasksRemaining); -#if HC_PLATFORM == HC_PLATFORM_GDK - if (m_appStateChangedToken) - { - UnregisterAppStateChangeNotification(m_appStateChangedToken); - m_appStateChangedToken = nullptr; - } -#endif - if (m_multiCleanupQueue) { XTaskQueueCloseHandle(m_multiCleanupQueue); } - // make sure XCurlMultis are cleaned up before curl_global_cleanup + // make sure CurlMultis are cleaned up before curl_global_cleanup m_curlMultis.clear(); -#if HC_PLATFORM == HC_PLATFORM_GDK - if (CurlDynamicLoader::GetInstance().IsLoaded()) - { - CURL_CALL(curl_global_cleanup)(); - } - // Free the dynamic loader singleton (unloads XCurl.dll via its destructor) - CurlDynamicLoader::DestroyInstance(); -#else - CURL_CALL(curl_global_cleanup)(); -#endif + curl_global_cleanup(); } HRESULT CurlProvider::PerformAsync(HCCallHandle hcCall, XAsyncBlock* async) noexcept { -#if HC_PLATFORM == HC_PLATFORM_GDK - // Check if curl is available before proceeding - if (!CurlDynamicLoader::GetInstance().IsLoaded()) - { - HC_TRACE_ERROR(HTTPCLIENT, "CurlProvider::PerformAsync: XCurl.dll not available"); - return E_HC_XCURL_REQUIRED; - } -#endif - XTaskQueuePortHandle workPort{ nullptr }; RETURN_IF_FAILED(XTaskQueueGetPort(async->queue, XTaskQueuePort::Work, &workPort)); @@ -250,87 +183,6 @@ void CALLBACK CurlProvider::MultiCleanupComplete(_Inout_ struct XAsyncBlock* asy } } -#if HC_PLATFORM == HC_PLATFORM_GDK - -// Total time the provider will spend driving curl_multi_perform while suspending. The xCurl -// contract requires the multi consumer to keep performing across suspend so xCurl can quiesce -// its handles; this bounds that work so a stuck request can never hold the suspend open longer -// than the platform's own watchdog budget. This is a budget for the WHOLE drain, shared across -// every CurlMulti, not a per-multi allowance -- a title with several task queues would otherwise -// multiply this by the number of multis and could still blow the suspend budget. -#define SUSPEND_DRAIN_TIMEOUT_MS 4000 - -void CurlProvider::Suspend() noexcept -{ - HC_TRACE_INFORMATION(HTTPCLIENT, "CurlProvider::Suspend"); - - // The lock is held for the whole drain so a concurrent CleanupAsync cannot move and destroy - // the CurlMultis while they are being performed. Completions are delivered through the - // async block's task queue rather than inline, so no request completion can re-enter - // PerformAsync on this thread while the lock is held. The drain is time-bounded, so the - // worst case for a blocked caller is SUSPEND_DRAIN_TIMEOUT_MS in total. - std::lock_guard lock{ m_mutex }; - - if (m_isSuspended) - { - return; - } - m_isSuspended = true; - - auto const deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(SUSPEND_DRAIN_TIMEOUT_MS); - - for (auto& pair : m_curlMultis) - { - CurlMulti* multi = pair.second.get(); - if (!multi || multi->ActiveRequestCount() == 0) - { - continue; - } - - // Give each multi only what is left of the shared budget so the total drain stays - // bounded regardless of how many multis (task queue work ports) exist. - auto const now = std::chrono::steady_clock::now(); - if (now >= deadline) - { - HC_TRACE_WARNING(HTTPCLIENT, "CurlProvider::Suspend: drain budget exhausted; remaining CurlMulti(s) not drained"); - break; - } - - auto const remainingMs = std::chrono::duration_cast(deadline - now).count(); - - HRESULT hr = multi->PerformUntilDrained(static_cast(remainingMs)); - if (FAILED(hr)) - { - HC_TRACE_WARNING_HR(HTTPCLIENT, hr, "CurlProvider::Suspend: CurlMulti did not fully drain before suspend"); - } - } -} - -void CurlProvider::Resume() noexcept -{ - HC_TRACE_INFORMATION(HTTPCLIENT, "CurlProvider::Resume"); - - std::lock_guard lock{ m_mutex }; - m_isSuspended = false; -} - -void CALLBACK CurlProvider::AppStateChangedCallback(BOOLEAN isSuspended, void* context) noexcept -{ - assert(context); - auto provider = static_cast(context); - - // RegisterAppStateChangeNotification reports "quiescing" as isSuspended == TRUE. - if (isSuspended) - { - provider->Suspend(); - } - else - { - provider->Resume(); - } -} - -#endif // HC_PLATFORM == HC_PLATFORM_GDK } // httpclient } // xbox diff --git a/Source/HTTP/Curl/CurlProvider.h b/Source/HTTP/Curl/CurlProvider.h index f919bb2a9..0f3267851 100644 --- a/Source/HTTP/Curl/CurlProvider.h +++ b/Source/HTTP/Curl/CurlProvider.h @@ -3,17 +3,8 @@ #include "Platform/IHttpProvider.h" #include "CurlMulti.h" #include "Result.h" -#if HC_PLATFORM == HC_PLATFORM_GDK -// When developing titles for Xbox consoles, you must use WinHTTP or xCurl. -// See https://docs.microsoft.com/en-us/gaming/gdk/_content/gc/networking/overviews/web-requests/http-networking for detail -#include -#include -#include "CurlDynamicLoader.h" -#else -// This http provider should work with other curl implementations as well. -// The logic in CurlMulti::Perform is optimized for XCurl, but should work on any curl implementation. +// This http provider is built for platforms that link libcurl directly. #include -#endif namespace xbox { @@ -39,28 +30,12 @@ struct CurlProvider : public IHttpProvider HRESULT CleanupAsync(XAsyncBlock* async) noexcept override; -#if HC_PLATFORM == HC_PLATFORM_GDK -public: // Suspend/resume handling (public so tests can drive it without real PLM transitions) - // Drives curl_multi_perform on this thread until outstanding requests drain, because the - // caller-supplied task queue that normally drives the perform loop may be parked while the - // title is suspended. See CurlMulti::PerformUntilDrained and bug 63050439. - void Suspend() noexcept; - void Resume() noexcept; -#endif - protected: CurlProvider() = default; static HRESULT CALLBACK CleanupAsyncProvider(XAsyncOp op, const XAsyncProviderData* data) noexcept; static void CALLBACK MultiCleanupComplete(_Inout_ struct XAsyncBlock* asyncBlock) noexcept; -#if HC_PLATFORM == HC_PLATFORM_GDK - static void CALLBACK AppStateChangedCallback(BOOLEAN isSuspended, void* context) noexcept; - - PAPPSTATE_REGISTRATION m_appStateChangedToken{ nullptr }; - bool m_isSuspended{ false }; -#endif - // Create an CurlMulti per work port http_internal_map> m_curlMultis{}; diff --git a/Source/HTTP/WinHttp/winhttp_connection.cpp b/Source/HTTP/WinHttp/winhttp_connection.cpp index 8cd3b4dae..1daa3aded 100644 --- a/Source/HTTP/WinHttp/winhttp_connection.cpp +++ b/Source/HTTP/WinHttp/winhttp_connection.cpp @@ -139,6 +139,17 @@ WinHttpConnection::~WinHttpConnection() } HCHttpCallCloseHandle(m_call); + + // Release this request's slot in the provider's global concurrency budget. Tied to object + // lifetime rather than complete_task because a connection can be torn down on paths that never + // complete the task (for example force-closed during suspend or provider shutdown); leaking a + // slot there would permanently shrink the budget until requests queued forever. + if (m_requestCompletedCallback) + { + auto callback = std::move(m_requestCompletedCallback); + m_requestCompletedCallback = nullptr; + callback(); + } } Result> WinHttpConnection::Initialize( @@ -1483,6 +1494,20 @@ void CALLBACK WinHttpConnection::completion_callback( { win32_cs_autolock cs{ &pRequestContext->m_lock }; pRequestContext->m_hRequest = nullptr; + + // Close the connect handle here rather than leaving it to the destructor. + // Callers that wait for this callback (PLM suspend) require every WinHTTP + // resource to be released before they destroy the session handles, and the + // destructor may not have run by then: m_connections holds only weak + // references, so the object's lifetime depends on refcounts that may still + // be held elsewhere. WinHttp guarantees no further callbacks for the + // request at this point, so the connect handle is safe to close. + if (pRequestContext->m_hConnection != nullptr) + { + WinHttpCloseHandle(pRequestContext->m_hConnection); + pRequestContext->m_hConnection = nullptr; + } + connectionClosedCallback = std::move(pRequestContext->m_connectionClosedCallback); pRequestContext->m_state = ConnectionState::Closed; } diff --git a/Source/HTTP/WinHttp/winhttp_connection.h b/Source/HTTP/WinHttp/winhttp_connection.h index 444e8600c..e95b80362 100644 --- a/Source/HTTP/WinHttp/winhttp_connection.h +++ b/Source/HTTP/WinHttp/winhttp_connection.h @@ -180,6 +180,10 @@ class WinHttpConnection : public std::enable_shared_from_this // Called by WinHttpProvider to force close on PLM Suspend or shutdown HRESULT Close(ConnectionClosedCallback callback); + // Invoked exactly once when the HTTP request completes, so WinHttpProvider can release the + // request's slot in the global concurrency budget and promote a queued request. Not used for + // WebSocket connections, which are long-lived and do not consume budget slots. + void SetRequestCompletedCallback(std::function callback) { m_requestCompletedCallback = std::move(callback); } private: WinHttpConnection( HINTERNET hSession, @@ -290,6 +294,7 @@ class WinHttpConnection : public std::enable_shared_from_this HCCallHandle m_call; // ref-counted, released in destructor Uri m_uri; XAsyncBlock* m_asyncBlock = nullptr; // non-owning + std::function m_requestCompletedCallback; XPlatSecurityInformation const m_securityInformation{}; ConnectionClosedCallback m_connectionClosedCallback; diff --git a/Source/HTTP/WinHttp/winhttp_provider.cpp b/Source/HTTP/WinHttp/winhttp_provider.cpp index 4c6c6ff8a..7f608ed75 100644 --- a/Source/HTTP/WinHttp/winhttp_provider.cpp +++ b/Source/HTTP/WinHttp/winhttp_provider.cpp @@ -1,5 +1,7 @@ #include "pch.h" #include "HTTP/httpcall.h" +#include "Global/global.h" +#include #include "winhttp_provider.h" #include "winhttp_connection.h" #include "uri.h" @@ -18,18 +20,7 @@ Result> WinHttpProvider::Initialize() RETURN_IF_FAILED(XTaskQueueCreate(XTaskQueueDispatchMode::Immediate, XTaskQueueDispatchMode::Immediate, &provider->m_immediateQueue)); #if HC_PLATFORM == HC_PLATFORM_GDK - if (XGameRuntimeIsFeatureAvailable(XGameRuntimeFeature::XNetworking)) - { - RETURN_IF_FAILED(XNetworkingRegisterConnectivityHintChanged(provider->m_immediateQueue, provider.get(), WinHttpProvider::NetworkConnectivityChangedCallback, &provider->m_networkConnectivityChangedToken)); - } - else - { - // XNetworking not available (e.g., PC GDK build), assume network is ready - provider->m_networkInitialized = true; - } - RETURN_IF_FAILED(RegisterAppStateChangeNotification(WinHttpProvider::AppStateChangedCallback, provider.get(), &provider->m_appStateChangedToken)); - #endif // HC_PLATFORM == HC_PLATFORM_GDK return std::move(provider); @@ -48,29 +39,36 @@ WinHttpProvider::~WinHttpProvider() UnregisterAppStateChangeNotification(m_appStateChangedToken); } - if (XGameRuntimeIsFeatureAvailable(XGameRuntimeFeature::XNetworking)) - { - if (m_networkConnectivityChangedToken.token) - { - XNetworkingUnregisterConnectivityHintChanged(m_networkConnectivityChangedToken, true); - } - } + // Unregistering above stops new PLM notifications, but one may already be executing. Take the + // suspend lock so teardown cannot run concurrently with an in-progress Suspend or Resume. + std::unique_lock suspendLock{ m_suspendLock }; #endif - HRESULT hr = CloseAllConnections(); + // INFINITE on the shutdown path: waiting for every connection to report closed is what keeps a + // WinHTTP callback from running after this object, the singleton, and any caller-supplied + // memory hooks are gone. + HRESULT hr = CloseAllConnections(INFINITE); if (FAILED(hr)) { HC_TRACE_ERROR_HR(HTTPCLIENT, hr, "WinHttpProvider::CloseAllConnections failed during shutdown"); } - for (auto& pair : m_hSessions) { - if (pair.second) + std::lock_guard lock{ m_lock }; + for (auto& pair : m_hSessions) { - WinHttpCloseHandle(pair.second); + if (pair.second) + { + WinHttpCloseHandle(pair.second); + } } + m_hSessions.clear(); } - m_hSessions.clear(); + +#if HC_PLATFORM == HC_PLATFORM_GDK + // Release before the mutex itself is destroyed with the object. + suspendLock.unlock(); +#endif } WinHttpWebSocketExports GetWinHttpWebSocketExportsHelper() @@ -99,6 +97,42 @@ HRESULT WinHttpProvider::PerformAsync( HCCallHandle callHandle, XAsyncBlock* async ) noexcept +{ + // Admission control: only GetGlobalRequestLimit() requests are allowed to reach WinHTTP at + // once, to bound the memory held by in-flight requests (each one costs WinHTTP request and + // connection handles, TLS state, and receive buffers). Requests beyond the cap are queued and + // started as earlier ones complete, so the caller never sees a failure for exceeding the cap. + uint64_t epoch{ 0 }; + { + std::lock_guard lock{ m_lock }; + + if (m_activeRequestCount >= GetGlobalRequestLimit()) + { + HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpProvider::PerformAsync queueing request, %u already active (limit %u)", + m_activeRequestCount, GetGlobalRequestLimit()); + m_pendingRequests.push_back(PendingRequest{ callHandle, async }); + return S_OK; + } + + ++m_activeRequestCount; + epoch = m_requestEpoch; + } + + HRESULT hr = StartRequest(callHandle, async, epoch); + if (FAILED(hr)) + { + // The request never reached WinHTTP, so it will never complete and would otherwise leak + // its slot. Release it and let any queued request take it. + OnRequestCompleted(epoch); + } + return hr; +} + +HRESULT WinHttpProvider::StartRequest( + HCCallHandle callHandle, + XAsyncBlock* async, + uint64_t epoch +) noexcept { // Get Security information for the call auto getSecurityInfoResult = GetSecurityInformation(callHandle->url.data()); @@ -110,7 +144,16 @@ HRESULT WinHttpProvider::PerformAsync( std::unique_lock lock{ m_lock }; #if HC_PLATFORM == HC_PLATFORM_GDK - if (!m_networkInitialized) + // Refuse new requests while suspended: PLM requires every network resource to be destroyed + // before the process is snapshotted, so nothing may be started until Resume. + // + // Note there is deliberately no "is the network initialized" check here. That gate used to + // consult a cached XNetworking connectivity hint, which could latch false permanently when the + // change notification was never delivered (observed on Steam Deck under Proton after an + // offline->online transition), leaving every request failing with E_HC_NETWORK_NOT_INITIALIZED + // for the life of the process. WinHTTP is the authority on whether a request can go out, so we + // let it try and return a real, retryable error instead. + if (m_isSuspended) { return E_HC_NETWORK_NOT_INITIALIZED; } @@ -123,8 +166,74 @@ HRESULT WinHttpProvider::PerformAsync( // Store weak reference to connection so we can close it if it is still active on shutdown m_connections.push_back(initConnectionResult.Payload()); + // Release the concurrency slot when the request finishes, however it finishes. Safe to capture + // `this`: the provider outlives its connections, which it force-closes in CloseAllConnections + // during both suspend and destruction. + initConnectionResult.Payload()->SetRequestCompletedCallback([this, epoch]() { OnRequestCompleted(epoch); }); + + // Unlock before starting: HttpCallPerformAsync can complete synchronously, which re-enters + // OnRequestCompleted and would deadlock on the non-recursive m_lock. + auto connection = initConnectionResult.Payload(); + lock.unlock(); + // WinHttpConnection manages its own lifetime from here - return initConnectionResult.Payload()->HttpCallPerformAsync(async); + return connection->HttpCallPerformAsync(async); +} + +void WinHttpProvider::OnRequestCompleted(uint64_t epoch) noexcept +{ + // Promote at most one queued request per completion, matching the slot that was just freed. + // Started outside the lock: HttpCallPerformAsync can complete synchronously and re-enter + // OnRequestCompleted, which would deadlock on the non-recursive m_lock. + for (;;) + { + PendingRequest next{}; + uint64_t nextEpoch{ 0 }; + { + std::lock_guard lock{ m_lock }; + + // CloseAllConnections already reclaimed every slot reserved in this epoch, so this + // release refers to a slot that no longer exists. Dropping it is what keeps the + // unsigned count from underflowing and wedging admission control permanently. + if (epoch != m_requestEpoch) + { + return; + } + + // Defensive: the epoch check above should make a zero count unreachable here, but the + // consequence of being wrong is a permanent stall, so saturate rather than wrap. + if (m_activeRequestCount > 0) + { + --m_activeRequestCount; + } + + if (m_pendingRequests.empty() || m_activeRequestCount >= GetGlobalRequestLimit()) + { + return; + } + + next = m_pendingRequests.front(); + m_pendingRequests.pop_front(); + ++m_activeRequestCount; + nextEpoch = m_requestEpoch; + } + + HRESULT hr = StartRequest(next.callHandle, next.async, nextEpoch); + if (SUCCEEDED(hr)) + { + return; + } + + // Complete the failed request for the caller, then loop to release its slot and give the + // next queued request a chance. Without this the caller would wait forever on a request + // that never reached WinHTTP. + HC_TRACE_ERROR_HR(HTTPCLIENT, hr, "WinHttpProvider: queued request failed to start"); + HCHttpCallResponseSetNetworkErrorCode(next.callHandle, hr, static_cast(hr)); + XAsyncComplete(next.async, S_OK, 0); + + // The next iteration releases the slot just reserved above, which belongs to nextEpoch. + epoch = nextEpoch; + } } HRESULT WinHttpProvider::SetGlobalProxy(_In_ String const& proxyUri) noexcept @@ -165,7 +274,9 @@ HRESULT WinHttpProvider::ConnectAsync( std::unique_lock lock{ m_lock }; #if HC_PLATFORM == HC_PLATFORM_GDK - if (!m_networkInitialized) + // See the equivalent comment in StartRequest: suspend blocks new work, but there is + // deliberately no cached network-initialized gate here. + if (m_isSuspended) { return E_HC_NETWORK_NOT_INITIALIZED; } @@ -230,7 +341,7 @@ HRESULT WinHttpProvider::Disconnect( } #endif //!HC_NOWEBSOCKETS -HRESULT WinHttpProvider::CloseAllConnections() +HRESULT WinHttpProvider::CloseAllConnections(DWORD timeoutMs) { // Should set result to HRESULT_FROM_WIN32(PROCESS_SUSPEND_RESUME) @@ -246,22 +357,26 @@ HRESULT WinHttpProvider::CloseAllConnections() HANDLE connectionsClosedEvent; std::atomic openConnections; + }; - } closeContext; + // Heap allocated and shared with the completion callback rather than living on this stack + // frame. The wait below is bounded, so a connection can report closed after this function has + // returned; a stack-allocated context would be freed memory by then. + auto closeContext = std::allocate_shared(http_stl_allocator{}); - closeContext.connectionsClosedEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr); - if (closeContext.connectionsClosedEvent == nullptr) + closeContext->connectionsClosedEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr); + if (closeContext->connectionsClosedEvent == nullptr) { return HRESULT_FROM_WIN32(GetLastError()); } - auto connectionClosedCallback = [&closeContext]() + auto connectionClosedCallback = [closeContext]() { - HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpProvider::Connection Closed, %llu remaining", closeContext.openConnections - 1); + HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpProvider::Connection Closed, %llu remaining", closeContext->openConnections - 1); - if (--closeContext.openConnections == 0) + if (--closeContext->openConnections == 0) { - SetEvent(closeContext.connectionsClosedEvent); + SetEvent(closeContext->connectionsClosedEvent); } }; @@ -280,14 +395,20 @@ HRESULT WinHttpProvider::CloseAllConnections() m_connections.clear(); } - closeContext.openConnections = connections.size(); - if (closeContext.openConnections > 0) + closeContext->openConnections = connections.size(); + if (closeContext->openConnections > 0) { for (auto& connection : connections) { assert(connection); if (connection) { + // Drop the budget callback before closing. These connections are about to be torn + // down as a group and the budget is reset in bulk below, so per-connection release + // is both unnecessary and unsafe here: a connection can outlive this call and would + // then invoke a callback capturing a provider that may already be destroyed. + connection->SetRequestCompletedCallback(nullptr); + HRESULT hr = connection->Close(connectionClosedCallback); if (FAILED(hr)) { @@ -295,7 +416,40 @@ HRESULT WinHttpProvider::CloseAllConnections() } } } - WaitForSingleObject(closeContext.connectionsClosedEvent, INFINITE); + // Bounded on the PLM suspend path, where the title has a short budget to comply before the + // OS terminates it: a connection that fails to report closed in time must not turn a slow + // teardown into a watchdog kill. The close context is shared with the callback, so a late + // report after this point is still safe. Shutdown passes INFINITE instead, because there + // the wait is what guarantees no WinHTTP callback can still run against connections that + // allocate through the libHttpClient allocator after the provider and singleton are gone. + if (WaitForSingleObject(closeContext->connectionsClosedEvent, timeoutMs) == WAIT_TIMEOUT) + { + HC_TRACE_ERROR(HTTPCLIENT, "WinHttpProvider::CloseAllConnections timed out after %ums with %llu connection(s) still open", + timeoutMs, static_cast(closeContext->openConnections.load())); + } + } + + // Every connection is gone, so nothing is in flight. Reset the budget rather than relying on + // per-request release, and fail anything still queued: those requests never reached WinHTTP and + // there is no longer a completion that would start them, so callers must not be left waiting. + http_internal_list abandoned; + { + std::lock_guard lock{ m_lock }; + m_activeRequestCount = 0; + + // Invalidate every slot reserved so far. A request that reserved a slot before this reset + // can still run its completion afterwards (it may have been mid-flight, or its connection + // may already have been destroyed and so skipped by the callback-clearing loop above); + // without this its release would decrement a count that was just zeroed and underflow it. + ++m_requestEpoch; + + abandoned.swap(m_pendingRequests); + } + + for (auto& pending : abandoned) + { + HCHttpCallResponseSetNetworkErrorCode(pending.callHandle, E_ABORT, static_cast(E_ABORT)); + XAsyncComplete(pending.async, S_OK, 0); } return S_OK; @@ -364,7 +518,8 @@ Result WinHttpProvider::GetHSession(uint32_t securityProtocolFlags, c #endif std::lock_guard lock(m_lock); - auto iter = m_hSessions.find(securityProtocolFlags); + SessionKey const sessionKey{ securityProtocolFlags, isHttps }; + auto iter = m_hSessions.find(sessionKey); if (iter != m_hSessions.end()) { HINTERNET hSession = iter->second; @@ -495,7 +650,7 @@ Result WinHttpProvider::GetHSession(uint32_t securityProtocolFlags, c (void)SetGlobalProxyForHSession(hSession, m_globalProxy.c_str()); } - m_hSessions[securityProtocolFlags] = hSession; + m_hSessions[sessionKey] = hSession; return hSession; } @@ -595,15 +750,25 @@ void WinHttpProvider::Suspend() { HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpProvider::Suspend"); + // Held across the entire sequence, including the blocking drain in CloseAllConnections, so a + // concurrent Resume or provider teardown cannot interleave with a suspend in progress. + std::lock_guard suspendLock{ m_suspendLock }; + { std::lock_guard lock{ m_lock }; - assert(!m_isSuspended); + if (m_isSuspended) + { + HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpProvider::Suspend called while already suspended, ignoring"); + return; + } m_isSuspended = true; - m_networkInitialized = false; } - HRESULT hr = CloseAllConnections(); + // Bounded on suspend: exceeding the platform's suspend budget gets the title killed by the + // watchdog, which is worse than proceeding with a connection that has not reported closed. + constexpr DWORD c_suspendCloseTimeoutMs = 2000; + HRESULT hr = CloseAllConnections(c_suspendCloseTimeoutMs); if (FAILED(hr)) { HC_TRACE_ERROR_HR(HTTPCLIENT, hr, "WinHttpProvider::CloseAllConnections failed during suspend, continuing with suspend sequence"); @@ -624,49 +789,18 @@ void WinHttpProvider::Resume() { HC_TRACE_INFORMATION(HTTPCLIENT, "WinHttpProvider::Resume"); - std::unique_lock lock{ m_lock }; + // Blocks until any in-progress Suspend has fully completed, so resume can never race ahead of + // the teardown and let new requests start against half-destroyed state. + std::lock_guard suspendLock{ m_suspendLock }; - assert(m_isSuspended); - m_isSuspended = false; - - lock.unlock(); - - // Force a query of network state since we've ignored notifications during suspend - NetworkConnectivityChangedCallback(this, nullptr); -} - -void WinHttpProvider::NetworkConnectivityChangedCallback(void* context, const XNetworkingConnectivityHint* /*hint*/) -{ - assert(context); - auto provider = static_cast(context); - - std::lock_guard lock{ provider->m_lock }; + std::lock_guard lock{ m_lock }; - // Ignore network connectivity changes if we are suspended - if (!provider->m_isSuspended) + if (!m_isSuspended) { - if (XGameRuntimeIsFeatureAvailable(XGameRuntimeFeature::XNetworking)) - { - // Always requery the latest network connectivity hint rather than relying on the passed parameter in case this is a stale notification - XNetworkingConnectivityHint hint{}; - HRESULT hr = XNetworkingGetConnectivityHint(&hint); - if (SUCCEEDED(hr)) - { - HC_TRACE_INFORMATION(HTTPCLIENT, "NetworkConnectivityChangedCallback, hint.networkInitialized=%d", hint.networkInitialized); - provider->m_networkInitialized = hint.networkInitialized; - } - else - { - HC_TRACE_ERROR(HTTPCLIENT, "Unable to get NetworkConnectivityHint, setting m_networkInitialized=false"); - provider->m_networkInitialized = false; - } - } - else - { - // Fallback to default network state if XNetworking is not available - provider->m_networkInitialized = true; - } + HC_TRACE_VERBOSE(HTTPCLIENT, "WinHttpProvider::Resume called while not suspended, ignoring"); + return; } + m_isSuspended = false; } void WinHttpProvider::AppStateChangedCallback(BOOLEAN isSuspended, void* context) diff --git a/Source/HTTP/WinHttp/winhttp_provider.h b/Source/HTTP/WinHttp/winhttp_provider.h index 5e5410ef4..78fe79eaf 100644 --- a/Source/HTTP/WinHttp/winhttp_provider.h +++ b/Source/HTTP/WinHttp/winhttp_provider.h @@ -70,6 +70,11 @@ class WinHttpProvider XAsyncBlock* async ) noexcept; + // Global cap on the number of HTTP requests allowed to be in flight against WinHTTP at once. + // Requests beyond the cap are queued and started as earlier requests complete, so callers may + // enqueue as many as they like. The limit itself is process-wide state owned by global.h + // (xbox::httpclient::GetGlobalRequestLimit), since it may be set before the provider exists. + HRESULT SetGlobalProxy( _In_ String const& proxyUri ) noexcept; @@ -108,7 +113,19 @@ class WinHttpProvider private: WinHttpProvider() = default; - HRESULT CloseAllConnections(); + // timeoutMs bounds the wait for connections to report closed. Suspend passes a short budget so + // a stuck connection cannot trigger the PLM watchdog; shutdown passes INFINITE because the wait + // is what prevents callbacks from outliving the provider. + HRESULT CloseAllConnections(DWORD timeoutMs); + + // Starts a request against WinHTTP immediately, bypassing the admission check. Callers must + // already hold a reserved slot in m_activeRequestCount, reserved during epoch `epoch`. + HRESULT StartRequest(HCCallHandle callHandle, XAsyncBlock* async, uint64_t epoch) noexcept; + + // Called when an admitted request finishes. Releases its slot and promotes queued requests. + // `epoch` is the value of m_requestEpoch when the slot was reserved; a release from an earlier + // epoch is ignored because CloseAllConnections already reclaimed those slots in bulk. + void OnRequestCompleted(uint64_t epoch) noexcept; Result GetSecurityInformation(const char* url); Result GetHSession(uint32_t securityProtocolFlags, const char* url); @@ -121,25 +138,69 @@ class WinHttpProvider http_internal_string m_globalProxy; std::mutex m_lock; - // Maintain a WinHttpSession for each unique security protocol flags - http_internal_map m_hSessions; + // Maintain a WinHttpSession for each unique (security protocol flags, secure scheme) pair. + // + // The scheme is part of the key because sessions are not interchangeable across schemes: + // GetHSession opens HTTPS sessions with WINHTTP_FLAG_SECURE_DEFAULTS, which permanently + // restricts that session to secure requests, and opens plain HTTP sessions with only + // WINHTTP_FLAG_ASYNC. Keying on the protocol flags alone let whichever scheme ran first win + // the cache slot, so an http:// or ws:// request that followed an https:// request reused the + // secure-defaults session and failed in WinHttpOpenRequest with ERROR_ACCESS_DENIED. + struct SessionKey + { + uint32_t securityProtocolFlags; + bool isSecure; + + bool operator<(SessionKey const& other) const + { + if (securityProtocolFlags != other.securityProtocolFlags) + { + return securityProtocolFlags < other.securityProtocolFlags; + } + return isSecure < other.isSecure; + } + }; + http_internal_map m_hSessions; // Track WinHttpConnections so that we can close them on shutdown/suspend http_internal_list> m_connections; + // Requests admitted to WinHTTP but not yet completed. Bounded by GetGlobalRequestLimit(). + uint32_t m_activeRequestCount{ 0 }; + + // Incremented whenever CloseAllConnections reclaims every slot at once. A slot reserved before + // that reset must not be released again afterwards: the count is unsigned, so the stray + // decrement would underflow to UINT32_MAX and make m_activeRequestCount >= the limit + // permanently true, queueing every subsequent request forever. Releases carry the epoch they + // were reserved in and are dropped if it no longer matches. + uint64_t m_requestEpoch{ 0 }; + + // Requests the caller has submitted that are waiting for a free slot. Unbounded by design: + // titles may queue as many requests as they like, only concurrency is capped. FIFO, so a + // queued request cannot be starved by later arrivals. + struct PendingRequest + { + HCCallHandle callHandle; + XAsyncBlock* async; + }; + http_internal_list m_pendingRequests; + #if HC_PLATFORM == HC_PLATFORM_GDK public: // For testing purposes only void Suspend(); void Resume(); private: - static void CALLBACK NetworkConnectivityChangedCallback(void* context, const XNetworkingConnectivityHint* hint); static void CALLBACK AppStateChangedCallback(BOOLEAN isSuspended, void* context); - bool m_networkInitialized{ false }; bool m_isSuspended{ false }; - XTaskQueueRegistrationToken m_networkConnectivityChangedToken{ 0 }; PAPPSTATE_REGISTRATION m_appStateChangedToken{ nullptr }; + + // Serializes the whole suspend sequence against resume and against provider destruction. + // Suspend drops m_lock while it blocks waiting for connections to close, so m_lock alone cannot + // keep a concurrent Resume (or a destructor running CloseAllConnections) from interleaving with + // a suspend that is still in progress. + std::mutex m_suspendLock; #endif }; diff --git a/Source/Platform/GDK/PlatformComponents_GDK.cpp b/Source/Platform/GDK/PlatformComponents_GDK.cpp index 2bc512b61..6a25ac13a 100644 --- a/Source/Platform/GDK/PlatformComponents_GDK.cpp +++ b/Source/Platform/GDK/PlatformComponents_GDK.cpp @@ -1,28 +1,12 @@ #include "pch.h" #include "Platform/PlatformComponents.h" -#include "HTTP/Curl/CurlProvider.h" #include "HTTP/WinHttp/winhttp_provider.h" #if !defined(HC_NOWEBSOCKETS) && defined(HC_ENABLE_WEBSOCKET_COMPRESSION) #include "HTTP/WinHttp/winhttp_websocket_hybrid.h" #endif -#if HC_PLATFORM == HC_PLATFORM_GDK -#include "XSystem.h" -#endif - NAMESPACE_XBOX_HTTP_CLIENT_BEGIN -// On GDK, desktop PC is the special case. Treat every non-PC device type as console -// so new console device types continue to take the safer console path by default. -static bool IsRunningOnXboxConsole() -{ -#if HC_PLATFORM == HC_PLATFORM_GDK - return XSystemGetDeviceType() != XSystemDeviceType::Pc; -#else - return false; -#endif -} - #ifndef HC_NOWEBSOCKETS static bool IsGdkXboxCompressionWebSocketProviderEnabled() noexcept { @@ -33,14 +17,13 @@ static bool IsGdkXboxCompressionWebSocketProviderEnabled() noexcept #endif } -static HRESULT InitializeGdkWebSocketProviders(PlatformComponents& components, bool enableCompressionWebSocketProvider) +// Selects the default WebSocket provider. Shares the caller's WinHttpProvider instance so the +// process has exactly one provider, and therefore one PLM registration and one suspend drain. +static HRESULT InitializeGdkWebSocketProviders( + PlatformComponents& components, + bool enableCompressionWebSocketProvider, + SharedPtr const& sharedWinHttpProvider) { - auto initWinHttpResult = WinHttpProvider::Initialize(); - RETURN_IF_FAILED(initWinHttpResult.hr); - - auto winHttpProvider = initWinHttpResult.ExtractPayload(); - auto sharedWinHttpProvider = SharedPtr{ winHttpProvider.release(), std::move(winHttpProvider.get_deleter()), http_stl_allocator{} }; - #if defined(HC_ENABLE_WEBSOCKET_COMPRESSION) if (enableCompressionWebSocketProvider) { @@ -61,50 +44,21 @@ HRESULT PlatformInitialize(PlatformComponents& components, HCInitArgs* initArgs) // We don't expect initArgs on GDK RETURN_HR_IF(E_INVALIDARG, initArgs); - // Detect runtime platform to choose appropriate HTTP provider - if (IsRunningOnXboxConsole()) - { - HC_TRACE_INFORMATION(HTTPCLIENT, "PlatformInitialize: Detected Xbox console, using XCurl for HTTP"); - - // Use XCurl for Xbox console with full PLM support - auto initXCurlResult = CurlProvider::Initialize(); - RETURN_IF_FAILED(initXCurlResult.hr); + HC_TRACE_INFORMATION(HTTPCLIENT, "PlatformInitialize: Using WinHTTP for HTTP"); - components.HttpProvider = initXCurlResult.ExtractPayload(); + auto initWinHttpResult = WinHttpProvider::Initialize(); + RETURN_IF_FAILED(initWinHttpResult.hr); -#ifndef HC_NOWEBSOCKETS - // For Xbox consoles with XCurl HTTP, still use WinHttp for the default WebSocket path. - bool const enableCompressionWebSocketProvider = IsGdkXboxCompressionWebSocketProviderEnabled(); - if (!enableCompressionWebSocketProvider) - { - HC_TRACE_INFORMATION(HTTPCLIENT, "PlatformInitialize: Xbox console compression WebSocket provider is disabled by build policy"); - } - RETURN_IF_FAILED(InitializeGdkWebSocketProviders(components, enableCompressionWebSocketProvider)); -#endif - } - else - { - HC_TRACE_INFORMATION(HTTPCLIENT, "PlatformInitialize: Detected non-console platform. Using WinHTTP for HTTP"); - - // Use WinHTTP for non-console platforms - auto initWinHttpResult = WinHttpProvider::Initialize(); - RETURN_IF_FAILED(initWinHttpResult.hr); + auto winHttpProvider = initWinHttpResult.ExtractPayload(); - auto winHttpProvider = initWinHttpResult.ExtractPayload(); + // Use the same WinHttpProvider instance for both HTTP and the default WebSocket path. + auto sharedWinHttpProvider = SharedPtr{ winHttpProvider.release(), std::move(winHttpProvider.get_deleter()), http_stl_allocator{} }; - // Use the same WinHttpProvider instance for both HTTP and the default WebSocket path. - auto sharedWinHttpProvider = SharedPtr{ winHttpProvider.release(), std::move(winHttpProvider.get_deleter()), http_stl_allocator{} }; - - components.HttpProvider = http_allocate_unique(sharedWinHttpProvider); + components.HttpProvider = http_allocate_unique(sharedWinHttpProvider); #ifndef HC_NOWEBSOCKETS -#if defined(HC_ENABLE_WEBSOCKET_COMPRESSION) - components.WebSocketProvider = http_allocate_unique(sharedWinHttpProvider); -#else - components.WebSocketProvider = http_allocate_unique(sharedWinHttpProvider); -#endif + RETURN_IF_FAILED(InitializeGdkWebSocketProviders(components, IsGdkXboxCompressionWebSocketProviderEnabled(), sharedWinHttpProvider)); #endif - } return S_OK; } diff --git a/Source/Platform/Linux/PlatformComponents_Linux.cpp b/Source/Platform/Linux/PlatformComponents_Linux.cpp index 34a35406e..17220d54d 100644 --- a/Source/Platform/Linux/PlatformComponents_Linux.cpp +++ b/Source/Platform/Linux/PlatformComponents_Linux.cpp @@ -10,11 +10,11 @@ HRESULT PlatformInitialize(PlatformComponents& components, HCInitArgs* initArgs) // We don't expect initArgs on linux RETURN_HR_IF(E_INVALIDARG, initArgs); - // XCurl will be used for HTTP - auto initXCurlResult = CurlProvider::Initialize(); - RETURN_IF_FAILED(initXCurlResult.hr); + // libcurl will be used for HTTP + auto initCurlResult = CurlProvider::Initialize(); + RETURN_IF_FAILED(initCurlResult.hr); - components.HttpProvider = initXCurlResult.ExtractPayload(); + components.HttpProvider = initCurlResult.ExtractPayload(); #ifndef HC_NOWEBSOCKETS // Websocketpp will be used for WebSockets diff --git a/Tests/UnitTests/Tests/AsyncBlockTests.cpp b/Tests/UnitTests/Tests/AsyncBlockTests.cpp index 6ba9af422..aa0c59622 100644 --- a/Tests/UnitTests/Tests/AsyncBlockTests.cpp +++ b/Tests/UnitTests/Tests/AsyncBlockTests.cpp @@ -399,9 +399,24 @@ DEFINE_TEST_CLASS(AsyncBlockTests) AsyncBlockTests::~AsyncBlockTests() { - VERIFY_ARE_EQUAL(s_AsyncLibGlobalStateCount, (DWORD)0); + // Completion of an async op (XAsyncGetStatus no longer returning E_PENDING) does not mean + // its AsyncState has been reclaimed yet: the final release happens on the queue thread + // slightly later. Poll rather than sampling the count immediately, otherwise this races + // with any test whose last operation completed just before it returned. + for (int i = 0; i < 500 && s_AsyncLibGlobalStateCount != 0; i++) + { + Sleep(10); + } + + DWORD leakedState = static_cast(s_AsyncLibGlobalStateCount.load()); + + // Tear the queue down before verifying. VERIFY_ARE_EQUAL throws on failure, so asserting + // first would skip this cleanup and leak the task queue, which then fails the global + // XTaskQueueUninitialize leak check in every subsequent TaskQueueTests case. XTaskQueueTerminate(queue, true, nullptr, nullptr); XTaskQueueCloseHandle(queue); + + VERIFY_ARE_EQUAL(leakedState, (DWORD)0); } DEFINE_TEST_CASE(VerifySimpleAsyncCall) diff --git a/Tests/UnitTests/Tests/GlobalTests.cpp b/Tests/UnitTests/Tests/GlobalTests.cpp index 0f5272442..9ca1d1636 100644 --- a/Tests/UnitTests/Tests/GlobalTests.cpp +++ b/Tests/UnitTests/Tests/GlobalTests.cpp @@ -296,6 +296,113 @@ DEFINE_TEST_CLASS(GlobalTests) HCCleanup(); XTaskQueueCloseHandle(queue); } + + // ------------------------------------------------------------------------ + // Global request limit + // + // These cover the API contract only. Enforcement (admission control and the pending + // queue) lives in WinHttpProvider, which this test project does not compile -- it builds + // PlatformComponents_Generic instead -- so concurrency behavior is covered by the + // device-side scenarios in PlayFab.C\Test\GameTestScenarios\lhc-xbox rather than here. + // ------------------------------------------------------------------------ + + // Restores the shipping default so a failure part-way through a test cannot leak a custom + // limit into later tests. The limit is process-wide and deliberately survives HCCleanup, + // so nothing else resets it. + struct RequestLimitRestorer + { + ~RequestLimitRestorer() { HCSettingsSetGlobalRequestLimit(0); } + }; + + DEFINE_TEST_CASE(TestGlobalRequestLimitDefault) + { + DEFINE_TEST_CASE_PROPERTIES(TestGlobalRequestLimitDefault); + RequestLimitRestorer restore; + + // Readable before HCInitialize: a title needs to size the cap before issuing requests, + // so these APIs must not require initialization. + VERIFY_ARE_EQUAL(HCIsInitialized(), false); + + uint32_t limit{ 0 }; + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(12u, limit); + } + + DEFINE_TEST_CASE(TestGlobalRequestLimitRoundTrip) + { + DEFINE_TEST_CASE_PROPERTIES(TestGlobalRequestLimitRoundTrip); + RequestLimitRestorer restore; + + VERIFY_SUCCEEDED(HCSettingsSetGlobalRequestLimit(4)); + + uint32_t limit{ 0 }; + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(4u, limit); + + VERIFY_SUCCEEDED(HCSettingsSetGlobalRequestLimit(64)); + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(64u, limit); + + // A single request in flight is a legitimate configuration, so 1 must be preserved + // rather than treated as a degenerate value. + VERIFY_SUCCEEDED(HCSettingsSetGlobalRequestLimit(1)); + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(1u, limit); + } + + DEFINE_TEST_CASE(TestGlobalRequestLimitZeroRestoresDefault) + { + DEFINE_TEST_CASE_PROPERTIES(TestGlobalRequestLimitZeroRestoresDefault); + RequestLimitRestorer restore; + + VERIFY_SUCCEEDED(HCSettingsSetGlobalRequestLimit(3)); + + // A literal cap of zero would park every request forever with no completion able to + // release a slot, wedging the title. 0 therefore means "restore the default". + VERIFY_SUCCEEDED(HCSettingsSetGlobalRequestLimit(0)); + + uint32_t limit{ 0 }; + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(12u, limit); + } + + DEFINE_TEST_CASE(TestGlobalRequestLimitInvalidArg) + { + DEFINE_TEST_CASE_PROPERTIES(TestGlobalRequestLimitInvalidArg); + + VERIFY_ARE_EQUAL(E_INVALIDARG, HCSettingsGetGlobalRequestLimit(nullptr)); + } + + DEFINE_TEST_CASE(TestGlobalRequestLimitSurvivesInitAndCleanup) + { + DEFINE_TEST_CASE_PROPERTIES(TestGlobalRequestLimitSurvivesInitAndCleanup); + RequestLimitRestorer restore; + + // Set before init: the provider must read the live global rather than snapshotting the + // value when it is constructed. + VERIFY_ARE_EQUAL(HCIsInitialized(), false); + VERIFY_SUCCEEDED(HCSettingsSetGlobalRequestLimit(7)); + + VERIFY_SUCCEEDED(HCInitialize(nullptr)); + + uint32_t limit{ 0 }; + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(7u, limit); + + // Settable while initialized too. + VERIFY_SUCCEEDED(HCSettingsSetGlobalRequestLimit(9)); + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(9u, limit); + + HCCleanup(); + + // Deliberately process-wide rather than part of http_singleton, so it outlives cleanup + // and a title that configures it once at startup does not have to re-apply it after + // every init/cleanup cycle. + VERIFY_ARE_EQUAL(HCIsInitialized(), false); + VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); + VERIFY_ARE_EQUAL(9u, limit); + } }; NAMESPACE_XBOX_HTTP_CLIENT_TEST_END diff --git a/libHttpClient.props b/libHttpClient.props index c8219d0cc..7ea5e88cd 100644 --- a/libHttpClient.props +++ b/libHttpClient.props @@ -90,13 +90,6 @@ - - - - - - - %(AdditionalLibraryDirectories);$(Console_SdkLibPath) From 838377ff58e3a0b5081071fb2865dbf62353f97b Mon Sep 17 00:00:00 2001 From: Jason Sandlin Date: Tue, 11 Aug 2026 13:23:33 -0700 Subject: [PATCH 2/6] Soft-fail PLM registration in WinHttpProvider::Initialize 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 --- Source/HTTP/WinHttp/winhttp_provider.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Source/HTTP/WinHttp/winhttp_provider.cpp b/Source/HTTP/WinHttp/winhttp_provider.cpp index 7f608ed75..9dc4e0057 100644 --- a/Source/HTTP/WinHttp/winhttp_provider.cpp +++ b/Source/HTTP/WinHttp/winhttp_provider.cpp @@ -20,7 +20,17 @@ Result> WinHttpProvider::Initialize() RETURN_IF_FAILED(XTaskQueueCreate(XTaskQueueDispatchMode::Immediate, XTaskQueueDispatchMode::Immediate, &provider->m_immediateQueue)); #if HC_PLATFORM == HC_PLATFORM_GDK - RETURN_IF_FAILED(RegisterAppStateChangeNotification(WinHttpProvider::AppStateChangedCallback, provider.get(), &provider->m_appStateChangedToken)); + // Soft-fail: PLM suspend handling is a resilience feature, so losing it must not cost the + // title HTTP entirely. RegisterAppStateChangeNotification resolves from + // api-ms-win-core-psm-appnotify, which is not guaranteed to be present in every configuration + // the GDK runs in. Log and continue without suspend notifications; the destructor tolerates a + // null token, and Suspend/Resume simply never fire. + HRESULT registerHr = RegisterAppStateChangeNotification(WinHttpProvider::AppStateChangedCallback, provider.get(), &provider->m_appStateChangedToken); + if (FAILED(registerHr)) + { + HC_TRACE_ERROR_HR(HTTPCLIENT, registerHr, "WinHttpProvider::Initialize: RegisterAppStateChangeNotification failed; suspend handling disabled"); + provider->m_appStateChangedToken = nullptr; + } #endif // HC_PLATFORM == HC_PLATFORM_GDK return std::move(provider); From c5e5ba5f38fe4d9ac765f26aecf8d1e9b925648a Mon Sep 17 00:00:00 2001 From: Jason Sandlin Date: Tue, 11 Aug 2026 14:28:40 -0700 Subject: [PATCH 3/6] Retain close contexts for connections that miss a bounded close 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 --- Source/HTTP/WinHttp/winhttp_provider.cpp | 63 ++++++++++++++++++------ Source/HTTP/WinHttp/winhttp_provider.h | 12 +++++ 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/Source/HTTP/WinHttp/winhttp_provider.cpp b/Source/HTTP/WinHttp/winhttp_provider.cpp index 9dc4e0057..1f55282e0 100644 --- a/Source/HTTP/WinHttp/winhttp_provider.cpp +++ b/Source/HTTP/WinHttp/winhttp_provider.cpp @@ -12,6 +12,20 @@ NAMESPACE_XBOX_HTTP_CLIENT_BEGIN +struct WinHttpProvider::CloseContext +{ + ~CloseContext() + { + if (connectionsClosedEvent) + { + CloseHandle(connectionsClosedEvent); + } + } + + HANDLE connectionsClosedEvent{ nullptr }; + std::atomic openConnections{ 0 }; +}; + Result> WinHttpProvider::Initialize() { http_stl_allocator a{}; @@ -355,20 +369,6 @@ HRESULT WinHttpProvider::CloseAllConnections(DWORD timeoutMs) { // Should set result to HRESULT_FROM_WIN32(PROCESS_SUSPEND_RESUME) - struct CloseContext - { - ~CloseContext() - { - if (connectionsClosedEvent) - { - CloseHandle(connectionsClosedEvent); - } - } - - HANDLE connectionsClosedEvent; - std::atomic openConnections; - }; - // Heap allocated and shared with the completion callback rather than living on this stack // frame. The wait below is bounded, so a connection can report closed after this function has // returned; a stack-allocated context would be freed memory by then. @@ -436,6 +436,41 @@ HRESULT WinHttpProvider::CloseAllConnections(DWORD timeoutMs) { HC_TRACE_ERROR(HTTPCLIENT, "WinHttpProvider::CloseAllConnections timed out after %ums with %llu connection(s) still open", timeoutMs, static_cast(closeContext->openConnections.load())); + + // Those connections have already been dropped from m_connections and cannot be closed + // again, so retain the context they were given. A later call - notably shutdown, which + // waits INFINITE - drains it below and so still observes them finishing. + std::lock_guard lock{ m_lock }; + m_pendingCloseContexts.push_back(closeContext); + } + } + + // Drain contexts left behind by earlier timed-out closes. + // + // Deliberately bounded even when the caller passed INFINITE. These connections already missed + // one close deadline, so they are exactly the ones most likely never to report at all; blocking + // shutdown on them forever would turn a rare stuck connection into a permanent hang in + // HCCleanup. The unbounded wait above still covers the connections this call closed itself, + // which is the case that actually protects the session handles the destructor is about to + // close. This is a best-effort second chance, not a guarantee. + constexpr DWORD c_retainedCloseTimeoutMs = 5000; + DWORD retainedTimeoutMs = (timeoutMs == INFINITE) ? c_retainedCloseTimeoutMs : timeoutMs; + + http_internal_vector> pendingCloseContexts; + { + std::lock_guard lock{ m_lock }; + pendingCloseContexts.swap(m_pendingCloseContexts); + } + + for (auto& pendingContext : pendingCloseContexts) + { + if (WaitForSingleObject(pendingContext->connectionsClosedEvent, retainedTimeoutMs) == WAIT_TIMEOUT) + { + HC_TRACE_ERROR(HTTPCLIENT, "WinHttpProvider::CloseAllConnections: %llu connection(s) from an earlier close still open", + static_cast(pendingContext->openConnections.load())); + + std::lock_guard lock{ m_lock }; + m_pendingCloseContexts.push_back(pendingContext); } } diff --git a/Source/HTTP/WinHttp/winhttp_provider.h b/Source/HTTP/WinHttp/winhttp_provider.h index 78fe79eaf..ed92acb29 100644 --- a/Source/HTTP/WinHttp/winhttp_provider.h +++ b/Source/HTTP/WinHttp/winhttp_provider.h @@ -163,6 +163,18 @@ class WinHttpProvider http_internal_map m_hSessions; // Track WinHttpConnections so that we can close them on shutdown/suspend + // Shared with the connection-closed callbacks so a connection that reports closed after a + // bounded wait has already returned still has a valid context to signal. Defined in the .cpp. + struct CloseContext; + + // Contexts from earlier CloseAllConnections calls whose bounded wait expired with connections + // still open. Those connections were dropped from m_connections and cannot be closed a second + // time (WinHttpConnection::Close is once-only and returns E_UNEXPECTED without invoking the + // callback), so waiting on the context they were originally given is the only way to observe + // them finishing. Shutdown drains these with an INFINITE timeout, which is what keeps a + // straggler from outliving the provider and the WinHTTP session handles it is still using. + http_internal_vector> m_pendingCloseContexts; + http_internal_list> m_connections; // Requests admitted to WinHTTP but not yet completed. Bounded by GetGlobalRequestLimit(). From 795af0b319140e9bffb163033e3348e2919a1828 Mon Sep 17 00:00:00 2001 From: Jason Sandlin Date: Tue, 11 Aug 2026 17:51:52 -0700 Subject: [PATCH 4/6] Address PR review feedback on request limit, exports and dead code - 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 --- .../libHttpClient.GDK.NoWebSockets.def | 4 +- Build/libHttpClient.GDK/libHttpClient.GDK.def | 5 +- .../libHttpClient.Win32.NoWebSockets.def | 4 +- .../libHttpClient.Win32.def | 4 +- Include/httpClient/httpClient.h | 27 ++++----- Source/Global/global.cpp | 37 +++++++++--- Source/Global/global_publics.cpp | 15 +++++ Source/HTTP/Curl/CurlMulti.cpp | 58 ------------------- Source/HTTP/Curl/CurlMulti.h | 17 ------ Source/HTTP/WinHttp/winhttp_provider.cpp | 54 +++++++++-------- Source/HTTP/WinHttp/winhttp_provider.h | 20 ++++--- Tests/UnitTests/Tests/GlobalTests.cpp | 8 ++- 12 files changed, 114 insertions(+), 139 deletions(-) diff --git a/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def b/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def index 115377aba..3af3b8fc9 100644 --- a/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def +++ b/Build/libHttpClient.GDK/libHttpClient.GDK.NoWebSockets.def @@ -84,8 +84,6 @@ EXPORTS HCMockSetMockMatchedCallback HCRemoveCallRoutedHandler HCSetGlobalProxy - HCSettingsSetGlobalRequestLimit - HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSettingsGetTraceLevel HCSettingsSetTraceLevel @@ -100,3 +98,5 @@ EXPORTS HCTraceSetTraceToDebugger HCWinHttpResume HCWinHttpSuspend + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit diff --git a/Build/libHttpClient.GDK/libHttpClient.GDK.def b/Build/libHttpClient.GDK/libHttpClient.GDK.def index 25565f4c9..a728ec3f8 100644 --- a/Build/libHttpClient.GDK/libHttpClient.GDK.def +++ b/Build/libHttpClient.GDK/libHttpClient.GDK.def @@ -85,8 +85,6 @@ EXPORTS HCRemoveCallRoutedHandler HCRemoveWebSocketRoutedHandler HCSetGlobalProxy - HCSettingsSetGlobalRequestLimit - HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSetWebSocketFunctions HCSettingsGetTraceLevel @@ -130,4 +128,5 @@ EXPORTS HCWebSocketSetPingInterval HCHttpCallRequestGetMaxReceiveBufferSize HCHttpCallRequestSetMaxReceiveBufferSize - + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit diff --git a/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def b/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def index bff95d8c0..dbd1cd548 100644 --- a/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def +++ b/Build/libHttpClient.Win32/libHttpClient.Win32.NoWebSockets.def @@ -79,8 +79,6 @@ EXPORTS HCMockSetMockMatchedCallback HCRemoveCallRoutedHandler HCSetGlobalProxy - HCSettingsSetGlobalRequestLimit - HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSettingsGetTraceLevel HCSettingsSetTraceLevel @@ -120,3 +118,5 @@ EXPORTS HCHttpCallRequestSetProgressReportFunction HCHttpCallRequestGetMaxReceiveBufferSize HCHttpCallRequestSetMaxReceiveBufferSize + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit diff --git a/Build/libHttpClient.Win32/libHttpClient.Win32.def b/Build/libHttpClient.Win32/libHttpClient.Win32.def index 16db9173c..7eef255a5 100644 --- a/Build/libHttpClient.Win32/libHttpClient.Win32.def +++ b/Build/libHttpClient.Win32/libHttpClient.Win32.def @@ -84,8 +84,6 @@ EXPORTS HCRemoveCallRoutedHandler HCRemoveWebSocketRoutedHandler HCSetGlobalProxy - HCSettingsSetGlobalRequestLimit - HCSettingsGetGlobalRequestLimit HCSetHttpCallPerformFunction HCSetWebSocketFunctions HCSettingsGetTraceLevel @@ -151,3 +149,5 @@ EXPORTS HCWebSocketSetPingInterval HCHttpCallRequestGetMaxReceiveBufferSize HCHttpCallRequestSetMaxReceiveBufferSize + HCSettingsSetGlobalRequestLimit + HCSettingsGetGlobalRequestLimit diff --git a/Include/httpClient/httpClient.h b/Include/httpClient/httpClient.h index abe896d9e..e61d32953 100644 --- a/Include/httpClient/httpClient.h +++ b/Include/httpClient/httpClient.h @@ -214,37 +214,38 @@ STDAPI HCSetGlobalProxy(_In_z_ const char* proxyUri) noexcept; /// /// Sets the maximum number of HTTP requests allowed to be in flight at one time. /// -/// The maximum number of concurrent HTTP requests. Passing 0 restores the default of 12. -/// Result code for this API operation. Possible values are S_OK, or E_FAIL. +/// The maximum number of concurrent HTTP requests. Passing 0 restores the device default. +/// Result code for this API operation. Possible values are S_OK, E_NOTIMPL, or E_FAIL. /// /// Requests submitted beyond this limit are queued and started automatically as earlier requests /// complete, so HCHttpCallPerformAsync() never fails because of the limit. Callers may create and /// submit as many HTTP calls as they like; only the number that reach the platform HTTP stack at /// once is capped. /// -/// The limit exists to bound the memory held by in-flight requests, which matters most on -/// memory-constrained titles. The default is 12. +/// The limit exists to bound the memory held by in-flight requests. It defaults to 12 on Xbox +/// consoles, where that budget is tightest, and is unlimited by default everywhere else, including +/// GDK on PC. The device is detected at runtime, so the same binary running on an Xbox console and +/// on PC will pick up different defaults. Titles that want a cap on PC should set one explicitly. /// /// This may be called before HCInitialize(). Changing the value does not affect requests that are /// already in flight, and lowering it will not cancel them; the count drains naturally as they /// complete. /// -/// Platform support: the limit is currently enforced only on GDK (Xbox and PC) and Win32, which -/// use the WinHTTP-based HTTP provider. On UWP, Linux, Android, iOS and macOS this value is stored -/// and returned by HCSettingsGetGlobalRequestLimit() but does not throttle requests, because those -/// platforms use HTTP providers that do not implement admission control. +/// Platform support: admission control is implemented by the WinHTTP-based HTTP provider, so the +/// limit is only supported on GDK (Xbox and PC) and Win32. On UWP, Linux, Android, iOS and macOS +/// this returns E_NOTIMPL rather than storing a value that would never throttle anything. /// STDAPI HCSettingsSetGlobalRequestLimit(_In_ uint32_t limit) noexcept; /// /// Gets the maximum number of HTTP requests allowed to be in flight at one time. /// -/// Passes back the current concurrent HTTP request limit. -/// Result code for this API operation. Possible values are S_OK, E_INVALIDARG, or E_FAIL. +/// Passes back the current concurrent HTTP request limit. 0xFFFFFFFF means unlimited. +/// Result code for this API operation. Possible values are S_OK, E_INVALIDARG, E_NOTIMPL, or E_FAIL. /// -/// This may be called before HCInitialize(). Returns the configured value on every platform, which -/// is not necessarily enforced on every platform - see HCSettingsSetGlobalRequestLimit() for the -/// list of platforms where the limit throttles requests. +/// This may be called before HCInitialize(). When no limit has been set explicitly this passes back +/// the device default, which is 0xFFFFFFFF (unlimited) on every device except Xbox consoles. Returns +/// E_NOTIMPL on platforms that do not support the limit - see HCSettingsSetGlobalRequestLimit(). /// STDAPI HCSettingsGetGlobalRequestLimit(_Out_ uint32_t* limit) noexcept; diff --git a/Source/Global/global.cpp b/Source/Global/global.cpp index 6999c8fcd..1a60dd7fc 100644 --- a/Source/Global/global.cpp +++ b/Source/Global/global.cpp @@ -8,6 +8,10 @@ #include "../Logger/trace_internal.h" #include "../Mock/lhc_mock.h" +#if HC_PLATFORM == HC_PLATFORM_GDK +#include "XSystem.h" +#endif + #ifndef HC_NOWEBSOCKETS #include "../WebSocket/hcwebsocket.h" #endif @@ -19,23 +23,40 @@ using namespace xbox::httpclient; NAMESPACE_XBOX_HTTP_CLIENT_BEGIN -// Bounds the memory held by in-flight requests, which matters most on memory-constrained titles. -// Applied uniformly on all platforms so a single shipped binary behaves identically everywhere. -constexpr uint32_t c_defaultGlobalRequestLimit = 12; +// Bounds the memory held by in-flight requests on Xbox consoles, where that budget is tightest. +// Every other device keeps the historical uncapped behavior unless a title opts in, so this change +// cannot regress concurrency for titles that were never throttled before. +constexpr uint32_t c_consoleDefaultGlobalRequestLimit = 12; +constexpr uint32_t c_unlimitedGlobalRequestLimit = UINT32_MAX; +// 0 means "no explicit limit configured"; GetGlobalRequestLimit resolves that to the device default. // Deliberately not part of http_singleton: this must be settable before HCInitialize. -static std::atomic g_globalRequestLimit{ c_defaultGlobalRequestLimit }; +static std::atomic g_globalRequestLimit{ 0 }; + +// Resolved per call rather than cached: the same Gaming.Desktop binary ships on Xbox consoles, GDK +// PC, Steam and Steam Deck, so the device is only knowable at runtime. Caching would also risk +// latching a wrong answer, because these APIs are callable before the game runtime is initialized. +static uint32_t DefaultGlobalRequestLimit() noexcept +{ +#if HC_PLATFORM == HC_PLATFORM_GDK + return XSystemGetDeviceType() == XSystemDeviceType::Pc + ? c_unlimitedGlobalRequestLimit + : c_consoleDefaultGlobalRequestLimit; +#else + return c_unlimitedGlobalRequestLimit; +#endif +} void SetGlobalRequestLimit(uint32_t limit) noexcept { - // A limit of 0 would stall every request forever with no way to recover, so treat it as - // "restore the default" rather than silently wedging the title. - g_globalRequestLimit.store(limit == 0 ? c_defaultGlobalRequestLimit : limit, std::memory_order_relaxed); + // Callers pass 0 to restore the default request limit. + g_globalRequestLimit.store(limit, std::memory_order_relaxed); } uint32_t GetGlobalRequestLimit() noexcept { - return g_globalRequestLimit.load(std::memory_order_relaxed); + uint32_t const limit = g_globalRequestLimit.load(std::memory_order_relaxed); + return limit == 0 ? DefaultGlobalRequestLimit() : limit; } HRESULT http_singleton::singleton_access( diff --git a/Source/Global/global_publics.cpp b/Source/Global/global_publics.cpp index 2629b82ab..3622bcd87 100644 --- a/Source/Global/global_publics.cpp +++ b/Source/Global/global_publics.cpp @@ -85,10 +85,18 @@ STDAPI HCSettingsSetGlobalRequestLimit(_In_ uint32_t limit) noexcept try { +#if HC_PLATFORM == HC_PLATFORM_WIN32 || HC_PLATFORM == HC_PLATFORM_GDK // Deliberately does not require initialization: the limit is process-wide state so it can be // configured before HCInitialize creates the provider. xbox::httpclient::SetGlobalRequestLimit(limit); return S_OK; +#else + // Admission control lives in the WinHTTP provider, which is only built for Win32 and GDK. + // Failing here is deliberate: storing a value that can never throttle anything would let a + // title believe it had configured a cap that silently does nothing. + (void)limit; + return E_NOTIMPL; +#endif } CATCH_RETURN() @@ -96,10 +104,17 @@ STDAPI HCSettingsGetGlobalRequestLimit(_Out_ uint32_t* limit) noexcept try { +#if HC_PLATFORM == HC_PLATFORM_WIN32 || HC_PLATFORM == HC_PLATFORM_GDK RETURN_HR_IF(E_INVALIDARG, !limit); *limit = xbox::httpclient::GetGlobalRequestLimit(); return S_OK; +#else + // Symmetric with HCSettingsSetGlobalRequestLimit: there is no meaningful limit to report on + // platforms whose HTTP provider does not implement admission control. + (void)limit; + return E_NOTIMPL; +#endif } CATCH_RETURN() diff --git a/Source/HTTP/Curl/CurlMulti.cpp b/Source/HTTP/Curl/CurlMulti.cpp index 35ec6e678..bc9d91f71 100644 --- a/Source/HTTP/Curl/CurlMulti.cpp +++ b/Source/HTTP/Curl/CurlMulti.cpp @@ -2,9 +2,6 @@ #include "CurlMulti.h" #include "CurlProvider.h" -#include -#include - namespace xbox { namespace httpclient @@ -275,61 +272,6 @@ HRESULT CurlMulti::Perform() noexcept return S_OK; } -size_t CurlMulti::ActiveRequestCount() noexcept -{ - std::lock_guard lock{ m_mutex }; - return m_easyRequests.size(); -} - -HRESULT CurlMulti::PerformUntilDrained(uint32_t timeoutMs) noexcept -{ - auto const deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); - - for (;;) - { - int runningRequests{ 0 }; - size_t activeRequests{ 0 }; - - { - std::unique_lock lock{ m_mutex }; - if (m_easyRequests.empty()) - { - return S_OK; - } - - HRESULT hr = PerformStepLocked(runningRequests); - if (FAILED(hr)) - { - // Match the task-queue path: an unexpected CURLM error fails everything rather - // than leaving requests wedged while the title is suspending. - lock.unlock(); - HC_TRACE_ERROR_HR(HTTPCLIENT, hr, "CurlMulti::PerformUntilDrained: Perform failed. Failing all active requests."); - FailAllRequests(hr); - return hr; - } - - activeRequests = m_easyRequests.size(); - } - - if (activeRequests == 0) - { - return S_OK; - } - - if (std::chrono::steady_clock::now() >= deadline) - { - HC_TRACE_WARNING(HTTPCLIENT, "CurlMulti::PerformUntilDrained: timed out with %zu request(s) still active", activeRequests); - // __HRESULT_FROM_WIN32 (not HRESULT_FROM_WIN32) because this file also builds for - // Linux/Android/Apple, where pal.h supplies the double-underscore form and the - // ERROR_TIMEOUT constant but not the single-underscore macro. - return __HRESULT_FROM_WIN32(ERROR_TIMEOUT); - } - - // Mirrors the delay the task queue path uses between curl_multi_perform calls. - std::this_thread::sleep_for(std::chrono::milliseconds(PERFORM_DELAY_MS)); - } -} - void CurlMulti::FailAllRequests(HRESULT hr) noexcept { std::unique_lock lock{ m_mutex }; diff --git a/Source/HTTP/Curl/CurlMulti.h b/Source/HTTP/Curl/CurlMulti.h index eec42b64c..84fef32db 100644 --- a/Source/HTTP/Curl/CurlMulti.h +++ b/Source/HTTP/Curl/CurlMulti.h @@ -20,23 +20,6 @@ class CurlMulti // Wrapper around curl_multi_add_handle HRESULT AddRequest(HC_UNIQUE_PTR easyRequest); - // Drives curl_multi_perform on the *calling* thread until every active request has - // completed or timeoutMs elapses. - // - // The normal perform loop only advances when the caller-supplied task queue is being - // dispatched (see ScheduleTaskQueueCallback), so this exists for callers that must make - // progress without that queue - originally the GDK suspend path for bug 63050439. - // - // NOTE: currently unreferenced. Its only caller was CurlProvider's GDK suspend handling, - // which was removed when GDK moved to the WinHTTP provider. Kept because it is provider - // -agnostic and the same need arises for any caller that must drain without the queue; - // delete it if no such caller materializes. - HRESULT PerformUntilDrained(uint32_t timeoutMs) noexcept; - - // Number of requests still owned by this multi handle. Currently only used by - // PerformUntilDrained. - size_t ActiveRequestCount() noexcept; - // Asyncronously cleanup any outstanding requests static HRESULT CleanupAsync(HC_UNIQUE_PTR multi, XAsyncBlock* async); diff --git a/Source/HTTP/WinHttp/winhttp_provider.cpp b/Source/HTTP/WinHttp/winhttp_provider.cpp index 1f55282e0..fdfa3ca7e 100644 --- a/Source/HTTP/WinHttp/winhttp_provider.cpp +++ b/Source/HTTP/WinHttp/winhttp_provider.cpp @@ -438,39 +438,47 @@ HRESULT WinHttpProvider::CloseAllConnections(DWORD timeoutMs) timeoutMs, static_cast(closeContext->openConnections.load())); // Those connections have already been dropped from m_connections and cannot be closed - // again, so retain the context they were given. A later call - notably shutdown, which - // waits INFINITE - drains it below and so still observes them finishing. + // again, so retain the context they were given. The shutdown path drains it below, so + // their eventual close is still observed rather than being lost entirely. std::lock_guard lock{ m_lock }; m_pendingCloseContexts.push_back(closeContext); } } - // Drain contexts left behind by earlier timed-out closes. + // Drain contexts left behind by earlier timed-out closes. Shutdown only. // - // Deliberately bounded even when the caller passed INFINITE. These connections already missed - // one close deadline, so they are exactly the ones most likely never to report at all; blocking - // shutdown on them forever would turn a rare stuck connection into a permanent hang in - // HCCleanup. The unbounded wait above still covers the connections this call closed itself, - // which is the case that actually protects the session handles the destructor is about to - // close. This is a best-effort second chance, not a guarantee. - constexpr DWORD c_retainedCloseTimeoutMs = 5000; - DWORD retainedTimeoutMs = (timeoutMs == INFINITE) ? c_retainedCloseTimeoutMs : timeoutMs; - - http_internal_vector> pendingCloseContexts; + // These exist to protect provider destruction: the destructor closes the WinHTTP session + // handles a straggler may still be using. Suspend destroys nothing, so draining there buys + // nothing and costs real time - each retained context would burn its full timeout in sequence, + // and these are by definition the connections least likely to ever report, so worst case adds + // N x the timeout to the suspend budget. That is precisely the watchdog kill the bounded wait + // above exists to avoid, and it would delay the E_ABORT completions below by the same amount. + // + // Bounded even here rather than INFINITE: a connection that already missed one deadline is the + // one most likely never to report, and blocking HCCleanup forever would be worse than the race + // this closes. The unbounded wait above still covers connections closed by this call, which is + // the case that actually protects the session handles. Best-effort second chance, not a + // guarantee. + if (timeoutMs == INFINITE) { - std::lock_guard lock{ m_lock }; - pendingCloseContexts.swap(m_pendingCloseContexts); - } + constexpr DWORD c_retainedCloseTimeoutMs = 5000; - for (auto& pendingContext : pendingCloseContexts) - { - if (WaitForSingleObject(pendingContext->connectionsClosedEvent, retainedTimeoutMs) == WAIT_TIMEOUT) + http_internal_vector> pendingCloseContexts; { - HC_TRACE_ERROR(HTTPCLIENT, "WinHttpProvider::CloseAllConnections: %llu connection(s) from an earlier close still open", - static_cast(pendingContext->openConnections.load())); - std::lock_guard lock{ m_lock }; - m_pendingCloseContexts.push_back(pendingContext); + pendingCloseContexts.swap(m_pendingCloseContexts); + } + + for (auto& pendingContext : pendingCloseContexts) + { + if (WaitForSingleObject(pendingContext->connectionsClosedEvent, c_retainedCloseTimeoutMs) == WAIT_TIMEOUT) + { + HC_TRACE_ERROR(HTTPCLIENT, "WinHttpProvider::CloseAllConnections: %llu connection(s) from an earlier close still open", + static_cast(pendingContext->openConnections.load())); + + std::lock_guard lock{ m_lock }; + m_pendingCloseContexts.push_back(pendingContext); + } } } diff --git a/Source/HTTP/WinHttp/winhttp_provider.h b/Source/HTTP/WinHttp/winhttp_provider.h index ed92acb29..b9fa41252 100644 --- a/Source/HTTP/WinHttp/winhttp_provider.h +++ b/Source/HTTP/WinHttp/winhttp_provider.h @@ -140,12 +140,12 @@ class WinHttpProvider // Maintain a WinHttpSession for each unique (security protocol flags, secure scheme) pair. // - // The scheme is part of the key because sessions are not interchangeable across schemes: - // GetHSession opens HTTPS sessions with WINHTTP_FLAG_SECURE_DEFAULTS, which permanently - // restricts that session to secure requests, and opens plain HTTP sessions with only - // WINHTTP_FLAG_ASYNC. Keying on the protocol flags alone let whichever scheme ran first win - // the cache slot, so an http:// or ws:// request that followed an https:// request reused the - // secure-defaults session and failed in WinHttpOpenRequest with ERROR_ACCESS_DENIED. + // `isSecure` is part of the key because WinHTTP sessions cannot be reused across TLS and + // non-TLS connections. TLS-enabled sessions are restricted with WINHTTP_FLAG_SECURE_DEFAULTS + // which then prevents those sessions from being used with insecure WS and HTTP schemes. + // Keying on the protocol flags alone let whichever ran first win the cache slot, so an http:// + // or ws:// request that followed an https:// request reused the secure-defaults session and + // failed in WinHttpOpenRequest with ERROR_ACCESS_DENIED. struct SessionKey { uint32_t securityProtocolFlags; @@ -162,7 +162,6 @@ class WinHttpProvider }; http_internal_map m_hSessions; - // Track WinHttpConnections so that we can close them on shutdown/suspend // Shared with the connection-closed callbacks so a connection that reports closed after a // bounded wait has already returned still has a valid context to signal. Defined in the .cpp. struct CloseContext; @@ -171,10 +170,13 @@ class WinHttpProvider // still open. Those connections were dropped from m_connections and cannot be closed a second // time (WinHttpConnection::Close is once-only and returns E_UNEXPECTED without invoking the // callback), so waiting on the context they were originally given is the only way to observe - // them finishing. Shutdown drains these with an INFINITE timeout, which is what keeps a - // straggler from outliving the provider and the WinHTTP session handles it is still using. + // them finishing. Only the shutdown path drains these, and only for a bounded time - not + // INFINITE - so a connection that already missed one deadline cannot hang HCCleanup forever. + // Best-effort: it narrows the window where a straggler outlives the provider and the WinHTTP + // session handles it is still using, rather than closing it entirely. http_internal_vector> m_pendingCloseContexts; + // Track WinHttpConnections so that we can close them on shutdown/suspend http_internal_list> m_connections; // Requests admitted to WinHTTP but not yet completed. Bounded by GetGlobalRequestLimit(). diff --git a/Tests/UnitTests/Tests/GlobalTests.cpp b/Tests/UnitTests/Tests/GlobalTests.cpp index 9ca1d1636..2eda1b8d3 100644 --- a/Tests/UnitTests/Tests/GlobalTests.cpp +++ b/Tests/UnitTests/Tests/GlobalTests.cpp @@ -314,6 +314,10 @@ DEFINE_TEST_CLASS(GlobalTests) ~RequestLimitRestorer() { HCSettingsSetGlobalRequestLimit(0); } }; + // The default is device-dependent: 12 on Xbox consoles, unlimited everywhere else. These unit + // tests build for Win32/UWP, so the expected default here is unlimited. + constexpr uint32_t c_expectedDefaultRequestLimit = UINT32_MAX; + DEFINE_TEST_CASE(TestGlobalRequestLimitDefault) { DEFINE_TEST_CASE_PROPERTIES(TestGlobalRequestLimitDefault); @@ -325,7 +329,7 @@ DEFINE_TEST_CLASS(GlobalTests) uint32_t limit{ 0 }; VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); - VERIFY_ARE_EQUAL(12u, limit); + VERIFY_ARE_EQUAL(c_expectedDefaultRequestLimit, limit); } DEFINE_TEST_CASE(TestGlobalRequestLimitRoundTrip) @@ -363,7 +367,7 @@ DEFINE_TEST_CLASS(GlobalTests) uint32_t limit{ 0 }; VERIFY_SUCCEEDED(HCSettingsGetGlobalRequestLimit(&limit)); - VERIFY_ARE_EQUAL(12u, limit); + VERIFY_ARE_EQUAL(c_expectedDefaultRequestLimit, limit); } DEFINE_TEST_CASE(TestGlobalRequestLimitInvalidArg) From 68a0a8dba25ca3e27036fb06206869ae872b5f8f Mon Sep 17 00:00:00 2001 From: Jason Sandlin Date: Tue, 11 Aug 2026 19:26:46 -0700 Subject: [PATCH 5/6] Fix C2178: move test constant to namespace scope 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 --- Tests/UnitTests/Tests/GlobalTests.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Tests/UnitTests/Tests/GlobalTests.cpp b/Tests/UnitTests/Tests/GlobalTests.cpp index 2eda1b8d3..3584f0cf1 100644 --- a/Tests/UnitTests/Tests/GlobalTests.cpp +++ b/Tests/UnitTests/Tests/GlobalTests.cpp @@ -36,6 +36,12 @@ static bool g_gotCall = false; NAMESPACE_XBOX_HTTP_CLIENT_TEST_BEGIN +// The default request limit is device-dependent: 12 on Xbox consoles, unlimited everywhere else. +// This test project builds for Win32/UWP, so the expected default here is unlimited. Kept at +// namespace scope rather than inside the test class because a non-static member cannot be +// constexpr, and a static one would be odr-used by VERIFY_ARE_EQUAL under C++14. +constexpr uint32_t c_expectedDefaultRequestLimit = UINT32_MAX; + DEFINE_TEST_CLASS(GlobalTests) { public: @@ -314,10 +320,6 @@ DEFINE_TEST_CLASS(GlobalTests) ~RequestLimitRestorer() { HCSettingsSetGlobalRequestLimit(0); } }; - // The default is device-dependent: 12 on Xbox consoles, unlimited everywhere else. These unit - // tests build for Win32/UWP, so the expected default here is unlimited. - constexpr uint32_t c_expectedDefaultRequestLimit = UINT32_MAX; - DEFINE_TEST_CASE(TestGlobalRequestLimitDefault) { DEFINE_TEST_CASE_PROPERTIES(TestGlobalRequestLimitDefault); From 07d1671de186cf3c72e84944aac1a680ea47b809 Mon Sep 17 00:00:00 2001 From: Jason Sandlin Date: Tue, 11 Aug 2026 20:16:14 -0700 Subject: [PATCH 6/6] Fix -Wunused-const-variable on non-GDK platforms 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 --- Source/Global/global.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Source/Global/global.cpp b/Source/Global/global.cpp index 1a60dd7fc..b89565002 100644 --- a/Source/Global/global.cpp +++ b/Source/Global/global.cpp @@ -23,10 +23,9 @@ using namespace xbox::httpclient; NAMESPACE_XBOX_HTTP_CLIENT_BEGIN -// Bounds the memory held by in-flight requests on Xbox consoles, where that budget is tightest. -// Every other device keeps the historical uncapped behavior unless a title opts in, so this change -// cannot regress concurrency for titles that were never throttled before. -constexpr uint32_t c_consoleDefaultGlobalRequestLimit = 12; +// Unlimited is the historical behavior on every platform, so it stays the default everywhere +// except Xbox consoles. That keeps this change from regressing concurrency for titles that were +// never throttled before. constexpr uint32_t c_unlimitedGlobalRequestLimit = UINT32_MAX; // 0 means "no explicit limit configured"; GetGlobalRequestLimit resolves that to the device default. @@ -39,6 +38,11 @@ static std::atomic g_globalRequestLimit{ 0 }; static uint32_t DefaultGlobalRequestLimit() noexcept { #if HC_PLATFORM == HC_PLATFORM_GDK + // Bounds the memory held by in-flight requests on Xbox consoles, where that budget is tightest. + // Declared here rather than at namespace scope so it is not an unused constant on the platforms + // that never consult it, which build with -Werror. + constexpr uint32_t c_consoleDefaultGlobalRequestLimit = 12; + return XSystemGetDeviceType() == XSystemDeviceType::Pc ? c_unlimitedGlobalRequestLimit : c_consoleDefaultGlobalRequestLimit;