Skip to content

Commit a84418b

Browse files
Fix cleanup deadlock with pending HTTP requests (#930) (#934)
* Fix cleanup deadlock caused by re-entrant lock during HTTP request cancellation When HCCleanupAsync cancels pending HTTP requests via XAsyncCancel(), the AsyncState destructor can be invoked synchronously, which attempts to re-acquire the NetworkState lock (m_mutex) that is already held by CleanupAsyncProvider. This manifests as a deadlock when cleanup is called with active HTTP requests that have pending retries. The fix snapshots the active requests while holding the lock, then releases the lock before calling XAsyncCancel() to avoid re-entrant lock acquisition. Also applies the same pattern to connected websocket disconnection for consistency. Fixes deadlock in TestAsyncCleanupWithHttpCallPendingRetry. * Use xbox:hc:Vector in fix for proper allocation hooking Previous commit used std::vector, which bypasses the LHC allocation hook --------- Co-authored-by: Jason Sandlin <jasonsa@microsoft.com>
1 parent b872e39 commit a84418b

1 file changed

Lines changed: 18 additions & 7 deletions

File tree

Source/Global/NetworkState.cpp

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -376,19 +376,31 @@ HRESULT CALLBACK NetworkState::CleanupAsyncProvider(XAsyncOp op, const XAsyncPro
376376
{
377377
case XAsyncOp::Begin:
378378
{
379-
std::unique_lock<std::mutex> lock{ state->m_mutex };
380-
state->m_cleanupAsyncBlock = data->async;
381-
bool scheduleCleanup = state->ScheduleCleanup();
379+
xbox::httpclient::Vector<XAsyncBlock*> activeRequests;
380+
#ifndef HC_NOWEBSOCKETS
381+
xbox::httpclient::Vector<ActiveWebSocketContext*> connectedWebSockets;
382+
#endif
383+
bool scheduleCleanup = false;
384+
{
385+
std::unique_lock<std::mutex> lock{ state->m_mutex };
386+
state->m_cleanupAsyncBlock = data->async;
387+
scheduleCleanup = state->ScheduleCleanup();
382388

389+
#ifndef HC_NOWEBSOCKETS
390+
HC_TRACE_VERBOSE(HTTPCLIENT, "NetworkState::CleanupAsyncProvider::Begin: HTTP active=%llu, WebSocket Connecting=%llu, WebSocket Connected=%llu", state->m_activeHttpRequests.size(), state->m_connectingWebSockets.size(), state->m_connectedWebSockets.size());
391+
#endif
392+
activeRequests.assign(state->m_activeHttpRequests.begin(), state->m_activeHttpRequests.end());
383393
#ifndef HC_NOWEBSOCKETS
384-
HC_TRACE_VERBOSE(HTTPCLIENT, "NetworkState::CleanupAsyncProvider::Begin: HTTP active=%llu, WebSocket Connecting=%llu, WebSocket Connected=%llu", state->m_activeHttpRequests.size(), state->m_connectingWebSockets.size(), state->m_connectedWebSockets.size());
394+
connectedWebSockets.assign(state->m_connectedWebSockets.begin(), state->m_connectedWebSockets.end());
385395
#endif
386-
for (auto& activeRequest : state->m_activeHttpRequests)
396+
}
397+
398+
for (auto& activeRequest : activeRequests)
387399
{
388400
XAsyncCancel(activeRequest);
389401
}
390402
#ifndef HC_NOWEBSOCKETS
391-
for (auto& context : state->m_connectedWebSockets)
403+
for (auto& context : connectedWebSockets)
392404
{
393405
HRESULT hr = context->websocketObserver->websocket->Disconnect();
394406
if (FAILED(hr))
@@ -397,7 +409,6 @@ HRESULT CALLBACK NetworkState::CleanupAsyncProvider(XAsyncOp op, const XAsyncPro
397409
}
398410
}
399411
#endif
400-
lock.unlock();
401412

402413
if (scheduleCleanup)
403414
{

0 commit comments

Comments
 (0)