From b23644fa3e6857699348d4112b6a07a66358ab29 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Wed, 9 Sep 2026 18:37:58 -0700 Subject: [PATCH] fix(http): enforce a hard deadline on WinHTTP by cancelling the request Setting WINHTTP_OPTION_RECEIVE_RESPONSE_TIMEOUT (previous commit) fixed the original failure -- the request is now always cancelled instead of waiting out a stall and returning 200 -- but the instrumented CI probe shows it is not cancelled on TIME. Five attempts with a 700ms budget against a server that accepts and then stalls 5s returned after 1490, 1529, 2485, 3493 and 4506ms, every one of them ERROR_WINHTTP_TIMEOUT (12002). One exceeded the test's 4s bound, which is why Windows CI was still red. That is the documented behaviour, not a mystery: both receive timeouts are "checked only when data is received from the socket", so an expired timeout is not surfaced until the peer sends something. Neither option is a deadline. It matters because `fetchSlots` is called synchronously on the OBS UI thread, behind the properties dialog's "Refresh camera list" button (obs-adapter/src/plugin-main.cpp:486, kPropertiesTimeoutMs = 5000). At the overshoot ratio measured above, a stalling server freezes that dialog for something like half a minute -- the exact failure the shortened timeout there was chosen to avoid. So: a watchdog thread that closes the request handle once the deadline passes, which is the documented way to cancel a WinHTTP operation. `RequestDeadline` owns the handle and both threads close it through an `atomic::exchange(nullptr)`, so exactly one close ever happens. Failures are reported as a timeout rather than as a raw GetLastError when the deadline is what fired. The ceiling is twice the caller's budget, not the budget itself: resolve, connect, send and receive each get `timeout` from WinHttpSetTimeouts, so a slow-but-progressing exchange can legitimately exceed one budget and must not be cancelled. There is one accepted race, documented at the class: the caller can load the handle just before the watchdog closes it, turning the call into ERROR_INVALID_HANDLE instead. Both mean the deadline expired. Cross-compiled with mingw-w64 (`-fsyntax-only`) rather than waiting on CI to find syntax errors; also confirmed by preprocessor probe that WINHTTP_OPTION_RECEIVE_RESPONSE_TIMEOUT is defined in those headers, so the #ifdef guard is not silently skipping the option. Linux: all 6 suites pass. Real verification is the Windows job's probe output. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzGnvQ6wfD7bw7PZN35ft9 --- core/src/http_winhttp.cpp | 137 +++++++++++++++++++++++++++++++------- 1 file changed, 114 insertions(+), 23 deletions(-) diff --git a/core/src/http_winhttp.cpp b/core/src/http_winhttp.cpp index 217406d..f479b44 100644 --- a/core/src/http_winhttp.cpp +++ b/core/src/http_winhttp.cpp @@ -19,8 +19,13 @@ You may obtain a copy of the License at #include #include +#include +#include +#include #include +#include #include +#include #include namespace stplugin { @@ -72,6 +77,79 @@ private: HINTERNET h_ = nullptr; }; +/// Hard deadline for one WinHTTP exchange, enforced by cancelling it. +/// +/// Neither receive timeout is a guaranteed deadline: Microsoft documents both +/// as "checked only when data is received from the socket", so an expired +/// timeout is not surfaced until the peer finally sends something. Measured on +/// the Windows CI runner against a server that accepts and then stalls 5s: a +/// 700ms budget returned after 1490, 1529, 2485, 3493 and 4506ms across five +/// attempts -- always cancelled, never on time. +/// +/// That overshoot matters because `fetchSlots` is called synchronously on the +/// OBS UI thread behind the properties dialog's "Refresh camera list" button +/// (obs-adapter/src/plugin-main.cpp), with a 5s budget. At the ratio above +/// that is a frozen dialog for half a minute. +/// +/// The documented way to force cancellation is to close the handle from +/// another thread; the pending call then fails with +/// ERROR_WINHTTP_OPERATION_CANCELLED. This owns the request handle so that +/// exactly one of the two threads ever closes it: `handle_.exchange(nullptr)` +/// hands the close to whichever gets there first. +/// +/// Known, accepted race: the caller may load the handle and have the watchdog +/// close it before the WinHttp* call reads it, in which case the call fails +/// with ERROR_INVALID_HANDLE instead. Both outcomes are "the deadline +/// expired", which is what the caller is told either way. +class RequestDeadline { +public: + RequestDeadline(HINTERNET request, DWORD after_ms) : handle_(request) + { + watchdog_ = std::thread([this, after_ms] { + std::unique_lock lock(mutex_); + if (cv_.wait_for(lock, std::chrono::milliseconds(after_ms), [this] { return finished_; })) + return; // exchange finished inside the deadline + if (closeOnce()) + expired_.store(true); + }); + } + + ~RequestDeadline() + { + { + std::lock_guard lock(mutex_); + finished_ = true; + } + cv_.notify_all(); + if (watchdog_.joinable()) + watchdog_.join(); + closeOnce(); // no-op if the watchdog got there first + } + + RequestDeadline(const RequestDeadline &) = delete; + RequestDeadline &operator=(const RequestDeadline &) = delete; + + HINTERNET get() const { return handle_.load(); } + bool expired() const { return expired_.load(); } + +private: + bool closeOnce() + { + HINTERNET h = handle_.exchange(nullptr); + if (!h) + return false; + WinHttpCloseHandle(h); + return true; + } + + std::atomic handle_; + std::atomic expired_{false}; + std::mutex mutex_; + std::condition_variable cv_; + bool finished_ = false; + std::thread watchdog_; +}; + class WinHttpClient : public HttpClient { public: HttpResponse send(const HttpRequest &request) override @@ -159,13 +237,36 @@ public: target += extra; const DWORD flags = (parts.nScheme == INTERNET_SCHEME_HTTPS) ? WINHTTP_FLAG_SECURE : 0u; - Handle req(WinHttpOpenRequest(connect.get(), widen(request.method).c_str(), target.c_str(), nullptr, - WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES, flags)); - if (!req) { + HINTERNET raw_req = WinHttpOpenRequest(connect.get(), widen(request.method).c_str(), target.c_str(), + nullptr, WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES, + flags); + if (!raw_req) { response.network_error = lastErrorMessage("WinHttpOpenRequest"); return response; } + // Ceiling at twice the caller's budget: each of the four + // WinHttpSetTimeouts phases (resolve, connect, send, receive) is + // allowed `timeout` on its own, so a slow-but-progressing exchange can + // legitimately exceed one budget, and this must not cancel those. The + // floor keeps a very small timeout_ms from producing a deadline the + // exchange cannot meet on a cold connection. + const DWORD deadline_ms = (timeout > 500u) ? (timeout * 2u) : 1000u; + RequestDeadline req(raw_req, deadline_ms); + + // From here on, `req.get()` can be closed underneath us by the + // watchdog; every WinHttp* failure below is therefore checked against + // req.expired() before its GetLastError text is reported, so an + // expired deadline reads as a timeout rather than as + // "WinHttpReceiveResponse failed (GetLastError=12017)". + const auto fail = [&](const char *what) -> HttpResponse { + if (req.expired()) + response.network_error = "timed out after " + std::to_string(deadline_ms) + " ms"; + else + response.network_error = lastErrorMessage(what); + return response; + }; + std::wstring headers; if (!request.content_type.empty()) headers = L"Content-Type: " + widen(request.content_type) + L"\r\n"; @@ -177,31 +278,23 @@ public: : const_cast(request.body.data()); const DWORD body_len = static_cast(request.body.size()); - if (!WinHttpSendRequest(req.get(), header_ptr, header_len, body_ptr, body_len, body_len, 0)) { - response.network_error = lastErrorMessage("WinHttpSendRequest"); - return response; - } - if (!WinHttpReceiveResponse(req.get(), nullptr)) { - response.network_error = lastErrorMessage("WinHttpReceiveResponse"); - return response; - } + if (!WinHttpSendRequest(req.get(), header_ptr, header_len, body_ptr, body_len, body_len, 0)) + return fail("WinHttpSendRequest"); + if (!WinHttpReceiveResponse(req.get(), nullptr)) + return fail("WinHttpReceiveResponse"); DWORD status = 0; DWORD status_size = sizeof(status); if (!WinHttpQueryHeaders(req.get(), WINHTTP_QUERY_STATUS_CODE | WINHTTP_QUERY_FLAG_NUMBER, - WINHTTP_HEADER_NAME_BY_INDEX, &status, &status_size, WINHTTP_NO_HEADER_INDEX)) { - response.network_error = lastErrorMessage("WinHttpQueryHeaders"); - return response; - } + WINHTTP_HEADER_NAME_BY_INDEX, &status, &status_size, WINHTTP_NO_HEADER_INDEX)) + return fail("WinHttpQueryHeaders"); response.status = static_cast(status); std::string body; for (;;) { DWORD available = 0; - if (!WinHttpQueryDataAvailable(req.get(), &available)) { - response.network_error = lastErrorMessage("WinHttpQueryDataAvailable"); - return response; - } + if (!WinHttpQueryDataAvailable(req.get(), &available)) + return fail("WinHttpQueryDataAvailable"); if (available == 0) break; if (body.size() + available > kMaxResponseBytes) { @@ -210,10 +303,8 @@ public: } std::vector chunk(available); DWORD read = 0; - if (!WinHttpReadData(req.get(), chunk.data(), available, &read)) { - response.network_error = lastErrorMessage("WinHttpReadData"); - return response; - } + if (!WinHttpReadData(req.get(), chunk.data(), available, &read)) + return fail("WinHttpReadData"); if (read == 0) break; body.append(chunk.data(), read);