Skip to content

Commit 28e0c10

Browse files
committed
Add repro for HTTP cleanup callback race
Add a unit-test-only NetworkState hook that reports whether cleanup can still cancel a request while the client's HTTP completion callback is running. Add TestHttpCallCompletionCallbackIsNotCleanupCancelable in GlobalTests to capture the current behavior with a mocked HTTP call. The test asserts that the request should no longer be cleanup-cancelable once the client callback is executing, which reproduces the lifetime bug that will be addressed in a follow-up fix commit.
1 parent efc01aa commit 28e0c10

File tree

3 files changed

+56
-0
lines changed

3 files changed

+56
-0
lines changed

Source/Global/NetworkState.cpp

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,21 @@ HRESULT NetworkState::HttpCallPerformAsync(HCCallHandle call, XAsyncBlock* async
101101
return S_OK;
102102
}
103103

104+
#ifdef HC_UNITTEST_API
105+
bool NetworkState::CanCleanupCancelHttpRequest(XAsyncBlock* async) noexcept
106+
{
107+
std::unique_lock<std::mutex> lock{ m_mutex };
108+
for (auto activeRequest : m_activeHttpRequests)
109+
{
110+
if (activeRequest == async)
111+
{
112+
return true;
113+
}
114+
}
115+
return false;
116+
}
117+
#endif
118+
104119
HRESULT CALLBACK NetworkState::HttpCallPerformAsyncProvider(XAsyncOp op, const XAsyncProviderData* data)
105120
{
106121
HttpPerformContext* performContext{ static_cast<HttpPerformContext*>(data->context) };

Source/Global/NetworkState.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ class NetworkState
4848
XAsyncBlock* async
4949
) noexcept;
5050

51+
#ifdef HC_UNITTEST_API
52+
bool CanCleanupCancelHttpRequest(XAsyncBlock* async) noexcept;
53+
#endif
54+
5155
#ifndef HC_NOWEBSOCKETS
5256
public: // WebSocket
5357
IWebSocketProvider& WebSocketProvider() noexcept;

Tests/UnitTests/Tests/GlobalTests.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,43 @@ DEFINE_TEST_CLASS(GlobalTests)
159159
VERIFY_ARE_EQUAL((DWORD)WAIT_OBJECT_0, WaitForSingleObject(cleanupCompleteEvent, INFINITE));
160160
CloseHandle(cleanupCompleteEvent);
161161
}
162+
163+
DEFINE_TEST_CASE(TestHttpCallCompletionCallbackIsNotCleanupCancelable)
164+
{
165+
VERIFY_SUCCEEDED(HCInitialize(nullptr));
166+
PumpedTaskQueue pumpedQueue;
167+
168+
constexpr char mockUrl[]{ "www.bing.com" };
169+
170+
HCMockCallHandle mock{ nullptr };
171+
VERIFY_SUCCEEDED(HCMockCallCreate(&mock));
172+
VERIFY_SUCCEEDED(HCMockResponseSetStatusCode(mock, 200));
173+
VERIFY_SUCCEEDED(HCMockAddMock(mock, "GET", mockUrl, nullptr, 0));
174+
175+
HCCallHandle call{ nullptr };
176+
VERIFY_SUCCEEDED(HCHttpCallCreate(&call));
177+
VERIFY_SUCCEEDED(HCHttpCallRequestSetUrl(call, "GET", mockUrl));
178+
179+
bool callbackSawCleanupCancelableRequest{ false };
180+
HANDLE httpCallCompleteEvent = CreateEvent(nullptr, FALSE, FALSE, nullptr);
181+
182+
XAsyncThunk httpPerformThunk{ [&](XAsyncBlock* async)
183+
{
184+
auto httpSingleton = get_http_singleton();
185+
VERIFY_IS_NOT_NULL(httpSingleton.get());
186+
callbackSawCleanupCancelableRequest = httpSingleton->m_networkState->CanCleanupCancelHttpRequest(async);
187+
SetEvent(httpCallCompleteEvent);
188+
}, pumpedQueue.queue };
189+
190+
VERIFY_SUCCEEDED(HCHttpCallPerformAsync(call, &httpPerformThunk.asyncBlock));
191+
VERIFY_ARE_EQUAL((DWORD)WAIT_OBJECT_0, WaitForSingleObject(httpCallCompleteEvent, INFINITE));
192+
193+
VERIFY_IS_FALSE(callbackSawCleanupCancelableRequest);
194+
195+
CloseHandle(httpCallCompleteEvent);
196+
VERIFY_SUCCEEDED(HCHttpCallCloseHandle(call));
197+
HCCleanup();
198+
}
162199
};
163200

164201
NAMESPACE_XBOX_HTTP_CLIENT_TEST_END

0 commit comments

Comments
 (0)