Azure / Azure/azure-sdk-for-cpp
[BUG] WinHttpRequest destructor blocks forever when the request is destroyed before `WinHttpSendRequest` binds the context
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 37
Description
**Library name and version:** Azure Core 1.16.3 (also present on `main` @ `426fb110f`)
## Describe the bug
`WinHttpRequest::~WinHttpRequest()` closes the request handle and then waits for
`WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING` before returning, so that no WinHTTP worker thread can
dereference the `WinHttpAction` after it is freed. That barrier is correct in principle, but it can
never be satisfied if the request is destroyed **before** `WinHttpSendRequest()` runs — and in that
case the calling thread blocks **permanently**.
The wait is unbounded and uncancellable, so the thread is lost for the lifetime of the process.
## Root cause
Three pieces of `sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp` interact:
**1. The status callback discards every notification that has no context:**
```cpp
void WinHttpAction::StatusCallback(
HINTERNET hInternet, DWORD_PTR dwContext, DWORD internetStatus, ...)
{
// If we're called before our context has been set (on Open and Close callbacks), ignore the
// status callback.
if (dwContext == 0)
{
return;
}
```
**2. The context is bound only by `WinHttpSendRequest()`, but the callback is registered in the
constructor** — so there is a window in which the handle has a callback but `dwContext == 0`:
```cpp
// WinHttpRequest::WinHttpRequest(), last statements of the constructor
m_httpAction = std::make_unique<_detail::WinHttpAction>(this);
if (!m_httpAction->RegisterWinHttpStatusCallback(m_requestHandle))
{
GetErrorAndThrow("Error while setting up the status callback.");
}
```
```cpp
// WinHttpRequest::SendRequest() - the only place the context is associated with the handle
WinHttpSendRequest(
m_requestHandle.get(), ..., reinterpret_cast(m_httpAction.get()));
```
**3. The destructor waits for a notification that step 1 will discard:**
```cpp
WinHttpRequest::~WinHttpRequest()
{
if (!m_requestHandleClosed)
{
Log::Write(Logger::Level::Informational,
"WinHttpRequest::~WinHttpRequest. Closing handle synchronously.");
if (!m_httpAction->WaitForAction(
[this]() { /* WinHttpCloseHandle(...) */ },
WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING,
Azure::Core::Context{})) // no deadline, never cancelled
```
`WaitForAction` loops on `WaitForSingleObject(..., pollDuration)` and only leaves the loop through
`context.ThrowIfCancelled()`, which is a no-op for a default-constructed `Context`. So when
`HANDLE_CLOSING` arrives with `dwContext == 0` and is dropped, `CompleteAction()` never runs, the
event is never signalled, and the loop spins forever.
The usual escape hatch is also closed: WinHTTP reports `WINHTTP_CALLBACK_STATUS_REQUEST_ERROR` /
`ERROR_WINHTTP_OPERATION_CANCELLED` when a handle with a pending operation is closed, but
`CompleteActionWithError()` deliberately does not signal while `m_expectedStatus ==
WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING`.
Anything that throws between the constructor completing and `WinHttpSendRequest()` being called
lands in this window. In `SendRequest()` that includes the header preparation
(`GetHeadersAsString()`, `StringToWideString()`) and `request.GetBodyStream()->Length()`.
We hit it in production under memory pressure, where an allocation during request setup threw
`std::bad_alloc` and permanently leaked the calling thread.
## Log signature
A healthy teardown always emits both lines within microseconds (this pairing is visible, for
example, in the CI logs attached to #5151):
```
DEBUG : WinHttpRequest::~WinHttpRequest. Closing handle synchronously.
INFO : Status operation: 2048(WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING )
DEBUG : Closing handle; completing outstanding Close request
INFO : WinHttpRequest::~WinHttpRequest. Handle closed.
```
A hung teardown emits only the first line, and the thread produces no further output ever again:
```
INFO : WinHttpRequest::~WinHttpRequest. Closing handle synchronously.
```
## To Reproduce
Deterministic, no network required, and reachable entirely through the public API.
`SendRequest()` calls `request.GetBodyStream()->Length()` before `WinHttpSendRequest()`, and
`BodyStream::Length()` is not `noexcept`, so a body stream that throws from `Length()` enters the
window exactly:
```cpp
class ThrowingLengthBodyStream final : public Azure::Core::IO::BodyStream {
public:
int64_t Length() const override { throw std::runtime_error("injected"); }
private:
size_t OnRead(uint8_t*, size_t, Azure::Core::Context const&) override { return 0; }
};
Azure::Core::Http::WinHttpTransport transport;
ThrowingLengthBodyStream bodyStream;
Azure::Core::Http::Request request(
Azure::Core::Http::HttpMethod::Put,
Azure::Core::Url("https://localhost/"),
&bodyStream);
Azure::Core::Context context;
transport.Send(request, context); // <-- never returns
```
`WinHttpOpen()` / `WinHttpConnect()` / `WinHttpOpenRequest()` only allocate handles, so nothing is
ever put on the wire and the URL does not need to resolve.
Observed: the call never returns. A leak checker additionally reports the abandoned
`WinHttpRequest` allocated in `WinHttpTransportImpl::CreateRequestHandle()`, which is the object
whose destructor is stuck.
## Expected behavior
Destroying a `WinHttpRequest` completes promptly regardless of how far the request progressed, and
`Send()` propagates the original failure.
## Suggested fix
Associate the context with the handle in the constructor, using
`WinHttpSetOption(WINHTTP_OPTION_CONTEXT_VALUE, ...)`, so `HANDLE_CLOSING` is always delivered with
a valid context. `WinHttpSendRequest()` then sets the same value idempotently and the existing
barrier keeps working in every state.
An alternative would be to track whether the context was ever bound and skip the wait when it was
not — safe for the same reason the bug exists (with no context, every callback is already dropped,
so there is nothing to synchronize against) — but it leaves `HANDLE_CLOSING` unobservable.
Note that simply adding a timeout to the destructor's wait would be **unsafe**: abandoning the wait
allows a WinHTTP worker thread to invoke the callback after `WinHttpAction` and `WinHttpRequest`
have been freed, turning a hang into a use-after-free.
## Related
- #6637 fixed a FailFast in this same function for the closely related case of "a request is
cancelled before the request is actually sent on the wire", and its comment already notes that
`WinHttpSendRequest` is what establishes the context. This issue is the hang that remains in that
window when the throw happens before `WaitForAction` is reached.
- #5151 contains CI logs showing the healthy `HANDLE_CLOSING` sequence for contrast.
## Setup
- OS: Windows Server 2022 / Windows 11
- Compiler: MSVC
- Transport: WinHTTP (`BUILD_TRANSPORT_WINHTTP_ADAPTER`)
- azure-core: 1.16.3 (vcpkg), and confirmed by inspection on `main` @ `426fb110f`
Contributor guide
Research direction
Start in sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp, tracing WinHttpRequest’s constructor, SendRequest(), destructor, and WinHttpAction::StatusCallback(). Run the ThrowingLengthBodyStream reproduction to observe the blocked teardown. Done means destruction completes promptly, the original Length() failure propagates, and the existing HANDLE_CLOSING synchronization remains safe.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100