fix(http): enforce a hard deadline on WinHTTP by cancelling the request
Build / macOS (macos-latest) (push) Successful in 51s
Build / Linux (ubuntu-24.04) (push) Successful in 1m14s
Build / macOS (macos-latest) (pull_request) Successful in 48s
Build / Linux (ubuntu-24.04) (pull_request) Successful in 1m8s
Build / Windows (windows-latest) (push) Successful in 3m56s
Build / Windows (windows-latest) (pull_request) Successful in 3m56s

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AzGnvQ6wfD7bw7PZN35ft9
This commit is contained in:
2026-09-09 18:37:58 -07:00
co-authored by Claude Opus 5
parent 8888e57d08
commit b23644fa3e
+114 -23
View File
@@ -19,8 +19,13 @@ You may obtain a copy of the License at
#include <windows.h> #include <windows.h>
#include <winhttp.h> #include <winhttp.h>
#include <atomic>
#include <chrono>
#include <condition_variable>
#include <cstddef> #include <cstddef>
#include <mutex>
#include <string> #include <string>
#include <thread>
#include <vector> #include <vector>
namespace stplugin { namespace stplugin {
@@ -72,6 +77,79 @@ private:
HINTERNET h_ = nullptr; 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<std::mutex> 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<std::mutex> 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<HINTERNET> handle_;
std::atomic<bool> expired_{false};
std::mutex mutex_;
std::condition_variable cv_;
bool finished_ = false;
std::thread watchdog_;
};
class WinHttpClient : public HttpClient { class WinHttpClient : public HttpClient {
public: public:
HttpResponse send(const HttpRequest &request) override HttpResponse send(const HttpRequest &request) override
@@ -159,13 +237,36 @@ public:
target += extra; target += extra;
const DWORD flags = (parts.nScheme == INTERNET_SCHEME_HTTPS) ? WINHTTP_FLAG_SECURE : 0u; 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, HINTERNET raw_req = WinHttpOpenRequest(connect.get(), widen(request.method).c_str(), target.c_str(),
WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES, flags)); nullptr, WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES,
if (!req) { flags);
if (!raw_req) {
response.network_error = lastErrorMessage("WinHttpOpenRequest"); response.network_error = lastErrorMessage("WinHttpOpenRequest");
return response; 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; std::wstring headers;
if (!request.content_type.empty()) if (!request.content_type.empty())
headers = L"Content-Type: " + widen(request.content_type) + L"\r\n"; headers = L"Content-Type: " + widen(request.content_type) + L"\r\n";
@@ -177,31 +278,23 @@ public:
: const_cast<char *>(request.body.data()); : const_cast<char *>(request.body.data());
const DWORD body_len = static_cast<DWORD>(request.body.size()); const DWORD body_len = static_cast<DWORD>(request.body.size());
if (!WinHttpSendRequest(req.get(), header_ptr, header_len, body_ptr, body_len, body_len, 0)) { if (!WinHttpSendRequest(req.get(), header_ptr, header_len, body_ptr, body_len, body_len, 0))
response.network_error = lastErrorMessage("WinHttpSendRequest"); return fail("WinHttpSendRequest");
return response; if (!WinHttpReceiveResponse(req.get(), nullptr))
} return fail("WinHttpReceiveResponse");
if (!WinHttpReceiveResponse(req.get(), nullptr)) {
response.network_error = lastErrorMessage("WinHttpReceiveResponse");
return response;
}
DWORD status = 0; DWORD status = 0;
DWORD status_size = sizeof(status); DWORD status_size = sizeof(status);
if (!WinHttpQueryHeaders(req.get(), WINHTTP_QUERY_STATUS_CODE | WINHTTP_QUERY_FLAG_NUMBER, if (!WinHttpQueryHeaders(req.get(), WINHTTP_QUERY_STATUS_CODE | WINHTTP_QUERY_FLAG_NUMBER,
WINHTTP_HEADER_NAME_BY_INDEX, &status, &status_size, WINHTTP_NO_HEADER_INDEX)) { WINHTTP_HEADER_NAME_BY_INDEX, &status, &status_size, WINHTTP_NO_HEADER_INDEX))
response.network_error = lastErrorMessage("WinHttpQueryHeaders"); return fail("WinHttpQueryHeaders");
return response;
}
response.status = static_cast<long>(status); response.status = static_cast<long>(status);
std::string body; std::string body;
for (;;) { for (;;) {
DWORD available = 0; DWORD available = 0;
if (!WinHttpQueryDataAvailable(req.get(), &available)) { if (!WinHttpQueryDataAvailable(req.get(), &available))
response.network_error = lastErrorMessage("WinHttpQueryDataAvailable"); return fail("WinHttpQueryDataAvailable");
return response;
}
if (available == 0) if (available == 0)
break; break;
if (body.size() + available > kMaxResponseBytes) { if (body.size() + available > kMaxResponseBytes) {
@@ -210,10 +303,8 @@ public:
} }
std::vector<char> chunk(available); std::vector<char> chunk(available);
DWORD read = 0; DWORD read = 0;
if (!WinHttpReadData(req.get(), chunk.data(), available, &read)) { if (!WinHttpReadData(req.get(), chunk.data(), available, &read))
response.network_error = lastErrorMessage("WinHttpReadData"); return fail("WinHttpReadData");
return response;
}
if (read == 0) if (read == 0)
break; break;
body.append(chunk.data(), read); body.append(chunk.data(), read);