diff --git a/Build/libHttpClient.Linux/CMakeLists.txt b/Build/libHttpClient.Linux/CMakeLists.txt index 15105c71..93e4b797 100644 --- a/Build/libHttpClient.Linux/CMakeLists.txt +++ b/Build/libHttpClient.Linux/CMakeLists.txt @@ -273,7 +273,7 @@ find_package(Threads REQUIRED) add_executable( "TaskQueueStarvationTests.Linux" - "${PATH_TO_ROOT}/Tests/TaskQueueStarvation/TaskQueueStarvationRepro.cpp" + "${PATH_TO_ROOT}/Tests/StandaloneTests/TaskQueueStarvationRepro.cpp" ) target_include_directories( @@ -330,7 +330,7 @@ set_tests_properties( # the libHttpClient.Linux target. add_executable( "SingleQueuePollStrandTests.Linux" - "${PATH_TO_ROOT}/Tests/TaskQueueStarvation/SingleQueuePollStrandRepro.cpp" + "${PATH_TO_ROOT}/Tests/StandaloneTests/SingleQueuePollStrandRepro.cpp" ) target_include_directories( @@ -376,4 +376,169 @@ set_tests_properties( LABELS "taskqueue" ) +################################################# +### WaitTimer queue-teardown race repro ### +################################################# +# This white-box repro links the normal library for its dependencies but +# compiles the private STL timer implementation into the executable so it can +# deterministically pause the worker after Pop(). The fix makes dispatch state +# independent from WaitTimerImpl lifetime, so a paused worker can no longer +# observe a destroyed timer. +add_executable( + "WaitTimerQueueTeardownTests.Linux" + "${PATH_TO_ROOT}/Tests/StandaloneTests/WaitTimerQueueTeardownRepro.cpp" + "${PATH_TO_ROOT}/Tests/StandaloneTests/WaitTimerTestMemory.cpp" + "${PATH_TO_ROOT}/Source/Task/WaitTimer_stl.cpp" +) + +target_include_directories( + "WaitTimerQueueTeardownTests.Linux" + PRIVATE + "${COMMON_INCLUDE_DIRS}" + "${LINUX_INCLUDE_DIRS}" +) + +target_link_libraries( + "WaitTimerQueueTeardownTests.Linux" + PRIVATE + "${PROJECT_NAME}" + Threads::Threads + ${CMAKE_DL_LIBS} +) + +if (NOT BUILD_SHARED_LIBS) + target_link_libraries( + "WaitTimerQueueTeardownTests.Linux" + PRIVATE + "${LIBCURL_BINARY_PATH}" + "${LIBSSL_BINARY_PATH}" + "${LIBCRYPTO_BINARY_PATH}" + ) +endif() + +target_set_flags( + "WaitTimerQueueTeardownTests.Linux" + "${FLAGS}" + "${FLAGS_DEBUG}" + "${FLAGS_RELEASE}" +) + +add_test( + NAME waittimer-queue-teardown-linux + COMMAND "WaitTimerQueueTeardownTests.Linux" +) +set_tests_properties( + waittimer-queue-teardown-linux + PROPERTIES + LABELS "taskqueue" +) + +################################################# +### WaitTimer queue-retirement race repro ### +################################################# +# White-box regression for the shared TimerQueue's last-owner retirement. It +# compiles the private STL timer implementation so it can pause inside +# RemoveTimer while the retiring owner holds g_timerQueueMutex, proving a +# concurrent Initialize cannot adopt a queue that is being retired. +add_executable( + "WaitTimerQueueRetirementTests.Linux" + "${PATH_TO_ROOT}/Tests/StandaloneTests/WaitTimerQueueRetirementRepro.cpp" + "${PATH_TO_ROOT}/Tests/StandaloneTests/WaitTimerTestMemory.cpp" + "${PATH_TO_ROOT}/Source/Task/WaitTimer_stl.cpp" +) + +target_include_directories( + "WaitTimerQueueRetirementTests.Linux" + PRIVATE + "${COMMON_INCLUDE_DIRS}" + "${LINUX_INCLUDE_DIRS}" +) + +target_link_libraries( + "WaitTimerQueueRetirementTests.Linux" + PRIVATE + "${PROJECT_NAME}" + Threads::Threads + ${CMAKE_DL_LIBS} +) + +if (NOT BUILD_SHARED_LIBS) + target_link_libraries( + "WaitTimerQueueRetirementTests.Linux" + PRIVATE + "${LIBCURL_BINARY_PATH}" + "${LIBSSL_BINARY_PATH}" + "${LIBCRYPTO_BINARY_PATH}" + ) +endif() + +target_set_flags( + "WaitTimerQueueRetirementTests.Linux" + "${FLAGS}" + "${FLAGS_DEBUG}" + "${FLAGS_RELEASE}" +) + +add_test( + NAME waittimer-queue-retirement-linux + COMMAND "WaitTimerQueueRetirementTests.Linux" +) +set_tests_properties( + waittimer-queue-retirement-linux + PROPERTIES + LABELS "taskqueue" +) + +################################################# +### WaitTimer wrapper lifetime race repro ### +################################################# +add_executable( + "WaitTimerWrapperTerminateTests.Linux" + "${PATH_TO_ROOT}/Tests/StandaloneTests/WaitTimerWrapperTerminateRepro.cpp" + "${PATH_TO_ROOT}/Tests/StandaloneTests/WaitTimerTestMemory.cpp" + "${PATH_TO_ROOT}/Source/Task/WaitTimer_stl.cpp" +) + +target_include_directories( + "WaitTimerWrapperTerminateTests.Linux" + PRIVATE + "${COMMON_INCLUDE_DIRS}" + "${LINUX_INCLUDE_DIRS}" +) + +target_link_libraries( + "WaitTimerWrapperTerminateTests.Linux" + PRIVATE + "${PROJECT_NAME}" + Threads::Threads + ${CMAKE_DL_LIBS} +) + +if (NOT BUILD_SHARED_LIBS) + target_link_libraries( + "WaitTimerWrapperTerminateTests.Linux" + PRIVATE + "${LIBCURL_BINARY_PATH}" + "${LIBSSL_BINARY_PATH}" + "${LIBCRYPTO_BINARY_PATH}" + ) +endif() + +target_set_flags( + "WaitTimerWrapperTerminateTests.Linux" + "${FLAGS}" + "${FLAGS_DEBUG}" + "${FLAGS_RELEASE}" +) + +add_test( + NAME waittimer-wrapper-terminate-linux + COMMAND "WaitTimerWrapperTerminateTests.Linux" +) +set_tests_properties( + waittimer-wrapper-terminate-linux + PROPERTIES + LABELS "taskqueue" +) + export(TARGETS ${PROJECT_NAME} FILE ${PROJECT_NAME}Config.cmake) diff --git a/Source/Task/WaitTimer.h b/Source/Task/WaitTimer.h index 0a80af5a..342a0880 100644 --- a/Source/Task/WaitTimer.h +++ b/Source/Task/WaitTimer.h @@ -1,11 +1,25 @@ #pragma once +#include + namespace OS { using WaitTimerCallback = void(_In_opt_ void*); class WaitTimerImpl; + // Source-internal test seam for deterministic STL WaitTimer lifetime + // regressions. It is not exported as part of the public libHttpClient API. + struct WaitTimerTestHooks + { + virtual void WaitTimerImplDestructionStarted() noexcept {} + virtual bool BeforeTimerInvoke() noexcept { return true; } + virtual void BeforeTimerQueueRetirement() noexcept {} + virtual bool WaitTimerOperationLoaded(bool) noexcept { return true; } + }; + + void WaitTimerSetTestHooks(_In_opt_ WaitTimerTestHooks* hooks) noexcept; + // A wait timer holds a single timeout expressed as a monotonic due time. // Calling Start will reset any pending timeout. class WaitTimer @@ -29,6 +43,7 @@ namespace OS uint64_t GetDueTime(_In_ uint32_t msFromNow) noexcept; private: + DefaultUnnamedMutex m_mutex; std::atomic m_impl; }; } diff --git a/Source/Task/WaitTimer_stl.cpp b/Source/Task/WaitTimer_stl.cpp index e4b61d5d..be4f8940 100644 --- a/Source/Task/WaitTimer_stl.cpp +++ b/Source/Task/WaitTimer_stl.cpp @@ -30,6 +30,65 @@ namespace OS { class TimerQueue; + class WaitTimerState + { + public: + WaitTimerState(_In_opt_ void* context, _In_ WaitTimerCallback* callback) noexcept + : m_context(context), m_callback(callback) + {} + + void BeginTerminate() noexcept + { + m_terminating.store(true, std::memory_order_release); + } + + bool TryBeginDispatch() noexcept + { + if (m_terminating.load(std::memory_order_acquire)) + { + return false; + } + + std::lock_guard lock{ m_mutex }; + if (m_terminating.load(std::memory_order_relaxed)) + { + return false; + } + + ++m_inFlightDispatch; + return true; + } + + void EndDispatch() noexcept + { + std::lock_guard lock{ m_mutex }; + ASSERT(m_inFlightDispatch != 0); + if (--m_inFlightDispatch == 0) + { + m_quiesced.notify_all(); + } + } + + void WaitForQuiesce() noexcept + { + std::unique_lock lock{ m_mutex }; + m_quiesced.wait(lock, [this]() noexcept { return m_inFlightDispatch == 0; }); + } + + void InvokeCallback() noexcept + { + m_callback(m_context); + } + + private: + void* m_context; + WaitTimerCallback* m_callback; + std::atomic m_terminating{ false }; + DefaultUnnamedMutex m_mutex; + DefaultUnnamedConditionVariable m_quiesced; + uint32_t m_inFlightDispatch = 0; + }; + class WaitTimerImpl { public: @@ -37,12 +96,10 @@ namespace OS HRESULT Initialize(_In_opt_ void* context, _In_ WaitTimerCallback* callback); void Start(_In_ uint64_t dueTime); void Cancel(); - void InvokeCallback(); + void Terminate() noexcept; private: - - void* m_context; - WaitTimerCallback* m_callback; + std::shared_ptr m_state; std::shared_ptr m_timerQueue; }; @@ -50,7 +107,9 @@ namespace OS { Deadline When; WaitTimerImpl* Timer; - TimerEntry(Deadline d, WaitTimerImpl* t) : When{ d }, Timer{ t } {} + std::shared_ptr State; + TimerEntry(Deadline d, WaitTimerImpl* timer, std::shared_ptr state) + : When{ d }, Timer{ timer }, State{ std::move(state) } {} }; struct TimerEntryComparator @@ -61,14 +120,23 @@ namespace OS } }; - class TimerQueue + // A single process-wide TimerQueue owns the worker thread that fires every + // WaitTimer callback. Ownership is shared: each live WaitTimerImpl holds a + // shared_ptr, and the running worker holds one too (captured in Init), so + // the queue is never destroyed while a callback is in flight. m_timerCount + // tracks live timers under g_timerQueueMutex so the last owner can retire + // the queue and stop the worker. + class TimerQueue : public std::enable_shared_from_this { public: bool Init() noexcept; ~TimerQueue(); - void Set(WaitTimerImpl* timer, Deadline deadline) noexcept; + void AddTimer() noexcept; + void RemoveTimer() noexcept; + void Set(WaitTimerImpl* timer, std::shared_ptr const& state, Deadline deadline) noexcept; void Remove(WaitTimerImpl const* timer) noexcept; + std::thread::id WorkerThreadId() const noexcept; private: void Worker() noexcept; @@ -80,6 +148,7 @@ namespace OS DefaultUnnamedConditionVariable m_cv; std::vector m_queue; // used as a heap std::thread m_t; + uint32_t m_timerCount = 0; // live timers; guarded by g_timerQueueMutex bool m_exitThread = false; bool m_initialized = false; }; @@ -88,19 +157,90 @@ namespace OS { std::shared_ptr g_timerQueue; DefaultUnnamedMutex g_timerQueueMutex; + DefaultUnnamedMutex g_testHooksMutex; + DefaultUnnamedConditionVariable g_testHooksChanged; + WaitTimerTestHooks* g_testHooks = nullptr; + std::atomic g_testHooksInstalled{ false }; + uint32_t g_testHooksActive = 0; + bool g_testHooksReplacing = false; + } + + class WaitTimerTestHookLease + { + public: + WaitTimerTestHookLease() noexcept + { + // Production fast path: when no hooks are installed, avoid the + // process-global hook mutex entirely on Start/Cancel/dispatch. + if (!g_testHooksInstalled.load(std::memory_order_acquire)) + { + return; + } + + std::unique_lock lock{ g_testHooksMutex }; + g_testHooksChanged.wait(lock, []() noexcept { return !g_testHooksReplacing; }); + m_hooks = g_testHooks; + if (m_hooks != nullptr) + { + ++g_testHooksActive; + } + } + + ~WaitTimerTestHookLease() + { + if (m_hooks != nullptr) + { + std::lock_guard lock{ g_testHooksMutex }; + ASSERT(g_testHooksActive != 0); + if (--g_testHooksActive == 0) + { + g_testHooksChanged.notify_all(); + } + } + } + + WaitTimerTestHooks* Get() const noexcept + { + return m_hooks; + } + + private: + WaitTimerTestHooks* m_hooks = nullptr; + }; + + void WaitTimerSetTestHooks(_In_opt_ WaitTimerTestHooks* hooks) noexcept + { + std::unique_lock lock{ g_testHooksMutex }; + g_testHooksReplacing = true; + g_testHooksChanged.wait(lock, []() noexcept { return g_testHooksActive == 0; }); + g_testHooks = hooks; + g_testHooksInstalled.store(hooks != nullptr, std::memory_order_release); + g_testHooksReplacing = false; + lock.unlock(); + g_testHooksChanged.notify_all(); } TimerQueue::~TimerQueue() { { - std::lock_guard lock{ m_mutex }; + std::lock_guard lock{ m_mutex }; m_exitThread = true; } m_cv.notify_all(); if (m_t.joinable()) { - m_t.join(); + // A timer callback can drop the last queue reference, so the queue + // may be destroyed on its own worker thread. Detach in that case; + // joining our own thread would deadlock. + if (m_t.get_id() == std::this_thread::get_id()) + { + m_t.detach(); + } + else + { + m_t.join(); + } } } @@ -110,9 +250,12 @@ namespace OS try { - m_t = std::thread([this]() + // Capture a shared_ptr so the worker keeps the queue alive for its + // whole lifetime. This lets a callback tear down its own timer (and + // the last queue reference) without the worker touching freed state. + m_t = std::thread([keepAlive = shared_from_this()]() { - Worker(); + keepAlive->Worker(); }); m_initialized = true; } @@ -124,20 +267,65 @@ namespace OS return m_initialized; } - void TimerQueue::Set(WaitTimerImpl* timer, Deadline deadline) noexcept + void TimerQueue::AddTimer() noexcept + { + // Caller holds g_timerQueueMutex, which serializes timer adoption in + // Initialize with last-owner retirement in RemoveTimer. + ++m_timerCount; + } + + void TimerQueue::RemoveTimer() noexcept + { + { + // The decrement and the last-owner decision run under the same lock + // Initialize takes to adopt a timer, so a queue that is retiring can + // never be resurrected by a concurrent Initialize. + std::lock_guard globalLock{ g_timerQueueMutex }; + ASSERT(m_timerCount != 0); + if (--m_timerCount != 0) + { + return; + } + + WaitTimerTestHookLease testHooks; + if (auto hooks = testHooks.Get()) + { + hooks->BeforeTimerQueueRetirement(); + } + + if (g_timerQueue.get() == this) + { + g_timerQueue.reset(); + } + } + + { + std::lock_guard lock{ m_mutex }; + m_exitThread = true; + } + m_cv.notify_all(); + } + + std::thread::id TimerQueue::WorkerThreadId() const noexcept + { + return m_t.get_id(); + } + + void TimerQueue::Set(WaitTimerImpl* timer, std::shared_ptr const& state, Deadline deadline) noexcept { { - std::lock_guard lock{ m_mutex }; + std::lock_guard lock{ m_mutex }; for (auto& entry : m_queue) { if (entry.Timer == timer) { entry.Timer = nullptr; + entry.State.reset(); } } - m_queue.emplace_back(deadline, timer); + m_queue.emplace_back(deadline, timer, state); std::push_heap(m_queue.begin(), m_queue.end(), TimerEntryComparator{}); } m_cv.notify_all(); @@ -145,7 +333,7 @@ namespace OS void TimerQueue::Remove(WaitTimerImpl const* timer) noexcept { - std::lock_guard lock{ m_mutex }; + std::lock_guard lock{ m_mutex }; // since m_queue is a heap, removing elements is non trivial, instead we // just clean the timer pointer and the entry will be popped eventually @@ -155,13 +343,14 @@ namespace OS if (entry.Timer == timer) { entry.Timer = nullptr; + entry.State.reset(); } } } void TimerQueue::Worker() noexcept { - std::unique_lock lock{ m_mutex }; + std::unique_lock lock{ m_mutex }; while (!m_exitThread) { while (!m_queue.empty()) @@ -174,12 +363,17 @@ namespace OS TimerEntry entry = Pop(); - // release the lock while invoking the callback, just in case timer - // gets destroyed on this thread or re-adds itself in the callback + // The raw timer pointer is only a cancellation key. Dispatch owns + // the shared state so teardown cannot invalidate the callback. lock.unlock(); - if (entry.Timer) // Timer is set to nullptr if the entry is removed + if (entry.State && entry.State->TryBeginDispatch()) { - entry.Timer->InvokeCallback(); + WaitTimerTestHookLease testHooks; + if (auto hooks = testHooks.Get(); hooks == nullptr || hooks->BeforeTimerInvoke()) + { + entry.State->InvokeCallback(); + } + entry.State->EndDispatch(); } lock.lock(); } @@ -213,27 +407,58 @@ namespace OS WaitTimerImpl::~WaitTimerImpl() { - std::lock_guard lock{ g_timerQueueMutex }; + Terminate(); + } + + void WaitTimerImpl::Terminate() noexcept + { + // Take ownership of the shared state and queue. Terminate is idempotent: + // the destructor always runs it, and a second call is a no-op once the + // members have been moved out. + std::shared_ptr state = std::move(m_state); + std::shared_ptr timerQueue = std::move(m_timerQueue); + + // Initialize may have failed before wiring up state/queue; there is + // nothing to tear down in that case. + if (state == nullptr || timerQueue == nullptr) + { + return; + } - // If we are the last one referencing the global timer the - // shared use count will be two (us + the global). If it is, - // clear out the global. We let our own reference reset - // as the class destructs. This puts it outside the mutex - // lock, which we want since there is some shutdown cost - // associated with shutting the timer down. + // Stop new dispatches, drop any queued entry, then wait for an in-flight + // dispatch to finish so the callback can never run against freed state. + // Skip the wait on the worker thread (a callback is tearing down its own + // timer): blocking on our own dispatch would deadlock, and the entry's + // shared-state lease keeps the state alive until the callback returns. + state->BeginTerminate(); + timerQueue->Remove(this); + if (std::this_thread::get_id() != timerQueue->WorkerThreadId()) + { + state->WaitForQuiesce(); + } - if (g_timerQueue.use_count() == 2) + WaitTimerTestHookLease testHooks; + if (auto hooks = testHooks.Get()) { - g_timerQueue.reset(); + hooks->WaitTimerImplDestructionStarted(); } + + // Release this timer's queue ownership; the last owner retires the queue. + timerQueue->RemoveTimer(); } HRESULT WaitTimerImpl::Initialize(_In_opt_ void* context, _In_ WaitTimerCallback* callback) { - m_context = context; - m_callback = callback; + try + { + m_state = http_allocate_shared(context, callback); + } + catch (const std::bad_alloc&) + { + return E_OUTOFMEMORY; + } - std::lock_guard lock{ g_timerQueueMutex }; + std::lock_guard lock{ g_timerQueueMutex }; if (g_timerQueue == nullptr) { @@ -254,13 +479,14 @@ namespace OS } m_timerQueue = g_timerQueue; + m_timerQueue->AddTimer(); return S_OK; } void WaitTimerImpl::Start(_In_ uint64_t dueTime) { - m_timerQueue->Set(this, Deadline(Deadline::duration(dueTime))); + m_timerQueue->Set(this, m_state, Deadline(Deadline::duration(dueTime))); } void WaitTimerImpl::Cancel() @@ -268,11 +494,6 @@ namespace OS m_timerQueue->Remove(this); } - void WaitTimerImpl::InvokeCallback() - { - m_callback(m_context); - } - WaitTimer::WaitTimer() noexcept : m_impl(nullptr) {} @@ -284,6 +505,7 @@ namespace OS HRESULT WaitTimer::Initialize(_In_opt_ void* context, _In_ WaitTimerCallback* callback) noexcept { + std::lock_guard lock{ m_mutex }; if (m_impl.load() != nullptr || callback == nullptr) { ASSERT(false); @@ -301,7 +523,11 @@ namespace OS void WaitTimer::Terminate() noexcept { - std::unique_ptr timer(m_impl.exchange(nullptr)); + std::unique_ptr timer; + { + std::lock_guard lock{ m_mutex }; + timer.reset(m_impl.exchange(nullptr)); + } if (timer != nullptr) { timer->Cancel(); @@ -310,12 +536,32 @@ namespace OS void WaitTimer::Start(_In_ uint64_t dueTime) noexcept { - m_impl.load()->Start(dueTime); + std::lock_guard lock{ m_mutex }; + WaitTimerImpl* timer = m_impl.load(); + WaitTimerTestHookLease testHooks; + if (auto hooks = testHooks.Get(); hooks != nullptr && !hooks->WaitTimerOperationLoaded(true)) + { + return; + } + if (timer != nullptr) + { + timer->Start(dueTime); + } } void WaitTimer::Cancel() noexcept { - m_impl.load()->Cancel(); + std::lock_guard lock{ m_mutex }; + WaitTimerImpl* timer = m_impl.load(); + WaitTimerTestHookLease testHooks; + if (auto hooks = testHooks.Get(); hooks != nullptr && !hooks->WaitTimerOperationLoaded(false)) + { + return; + } + if (timer != nullptr) + { + timer->Cancel(); + } } uint64_t WaitTimer::GetCurrentTime() noexcept diff --git a/Source/Task/WaitTimer_win32.cpp b/Source/Task/WaitTimer_win32.cpp index 716dcf16..8f97a6fa 100644 --- a/Source/Task/WaitTimer_win32.cpp +++ b/Source/Task/WaitTimer_win32.cpp @@ -126,6 +126,7 @@ namespace OS HRESULT WaitTimer::Initialize(_In_opt_ void* context, _In_ WaitTimerCallback* callback) noexcept { + std::lock_guard lock{ m_mutex }; if (m_impl.load() != nullptr || callback == nullptr) { ASSERT(FALSE); @@ -143,7 +144,11 @@ namespace OS void WaitTimer::Terminate() noexcept { - std::unique_ptr timer(m_impl.exchange(nullptr)); + std::unique_ptr timer; + { + std::lock_guard lock{ m_mutex }; + timer.reset(m_impl.exchange(nullptr)); + } if (timer != nullptr) { timer->Terminate(); @@ -152,12 +157,20 @@ namespace OS void WaitTimer::Start(_In_ uint64_t dueTime) noexcept { - m_impl.load()->Start(dueTime); + std::lock_guard lock{ m_mutex }; + if (WaitTimerImpl* timer = m_impl.load()) + { + timer->Start(dueTime); + } } void WaitTimer::Cancel() noexcept { - m_impl.load()->Cancel(); + std::lock_guard lock{ m_mutex }; + if (WaitTimerImpl* timer = m_impl.load()) + { + timer->Cancel(); + } } uint64_t WaitTimer::GetCurrentTime() noexcept diff --git a/Source/Task/iOS/ios_WaitTimer.mm b/Source/Task/iOS/ios_WaitTimer.mm index 53257ebe..c7d0409e 100644 --- a/Source/Task/iOS/ios_WaitTimer.mm +++ b/Source/Task/iOS/ios_WaitTimer.mm @@ -100,6 +100,7 @@ uint64_t DueTimeFromDeadline(Deadline deadline) noexcept HRESULT WaitTimer::Initialize(_In_opt_ void* context, _In_ WaitTimerCallback* callback) noexcept { + std::lock_guard lock{ m_mutex }; if (m_impl.load() != nullptr || callback == nullptr) { ASSERT(false); @@ -117,7 +118,11 @@ uint64_t DueTimeFromDeadline(Deadline deadline) noexcept void WaitTimer::Terminate() noexcept { - WaitTimerImpl* timer = m_impl.exchange(nullptr); + WaitTimerImpl* timer; + { + std::lock_guard lock{ m_mutex }; + timer = m_impl.exchange(nullptr); + } if (timer != nullptr) { timer->Cancel(); @@ -127,12 +132,20 @@ uint64_t DueTimeFromDeadline(Deadline deadline) noexcept void WaitTimer::Start(_In_ uint64_t dueTime) noexcept { - m_impl.load()->Start(dueTime); + std::lock_guard lock{ m_mutex }; + if (WaitTimerImpl* timer = m_impl.load()) + { + timer->Start(dueTime); + } } void WaitTimer::Cancel() noexcept { - m_impl.load()->Cancel(); + std::lock_guard lock{ m_mutex }; + if (WaitTimerImpl* timer = m_impl.load()) + { + timer->Cancel(); + } } uint64_t WaitTimer::GetCurrentTime() noexcept diff --git a/Tests/TaskQueueStarvation/SingleQueuePollStrandRepro.cpp b/Tests/StandaloneTests/SingleQueuePollStrandRepro.cpp similarity index 100% rename from Tests/TaskQueueStarvation/SingleQueuePollStrandRepro.cpp rename to Tests/StandaloneTests/SingleQueuePollStrandRepro.cpp diff --git a/Tests/TaskQueueStarvation/TaskQueueStarvationRepro.cpp b/Tests/StandaloneTests/TaskQueueStarvationRepro.cpp similarity index 100% rename from Tests/TaskQueueStarvation/TaskQueueStarvationRepro.cpp rename to Tests/StandaloneTests/TaskQueueStarvationRepro.cpp diff --git a/Tests/StandaloneTests/WaitTimerQueueRetirementRepro.cpp b/Tests/StandaloneTests/WaitTimerQueueRetirementRepro.cpp new file mode 100644 index 00000000..2bb682c1 --- /dev/null +++ b/Tests/StandaloneTests/WaitTimerQueueRetirementRepro.cpp @@ -0,0 +1,127 @@ +// Copyright (c) Microsoft Corporation +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// Deterministic regression for last-owner retirement racing a new timer. This +// target compiles Source/Task/WaitTimer_stl.cpp directly so it can pause inside +// TimerQueue::RemoveTimer, exactly while the retiring owner holds +// g_timerQueueMutex, and prove a concurrent Initialize cannot resurrect a queue +// that is being retired. + +#include +#include "../../Source/Task/WaitTimer.h" + +#include +#include +#include +#include +#include +#include + +namespace +{ + using namespace std::chrono_literals; + + class RetirementHooks final : public OS::WaitTimerTestHooks + { + public: + void BeforeTimerQueueRetirement() noexcept override + { + std::unique_lock lock(m_mutex); + m_retirementStarted = true; + m_changed.notify_all(); + m_changed.wait(lock, [this]() noexcept { return m_releaseRetirement; }); + } + + bool WaitForRetirement() + { + std::unique_lock lock(m_mutex); + return m_changed.wait_for(lock, 5s, [this]() noexcept { return m_retirementStarted; }); + } + + void ReleaseRetirement() + { + std::lock_guard lock(m_mutex); + m_releaseRetirement = true; + m_changed.notify_all(); + } + + private: + std::mutex m_mutex; + std::condition_variable m_changed; + bool m_retirementStarted = false; + bool m_releaseRetirement = false; + }; + + struct CallbackState + { + std::mutex Mutex; + std::condition_variable Changed; + bool Ran = false; + }; + + void CALLBACK TimerCallback(void* context) + { + auto callbackState = static_cast(context); + { + std::lock_guard lock(callbackState->Mutex); + callbackState->Ran = true; + } + callbackState->Changed.notify_all(); + } +} + +int main() +{ + RetirementHooks hooks; + OS::WaitTimerSetTestHooks(&hooks); + + OS::WaitTimer retiringTimer; + HRESULT hr = retiringTimer.Initialize(nullptr, [](void*) {}); + if (FAILED(hr)) + { + std::printf("[waittimer-queue-retirement] FAILED: first Initialize returned 0x%08x\n", static_cast(hr)); + OS::WaitTimerSetTestHooks(nullptr); + return 2; + } + + std::thread retire([&retiringTimer]() { retiringTimer.Terminate(); }); + if (!hooks.WaitForRetirement()) + { + std::printf("[waittimer-queue-retirement] FAILED: timer queue did not begin last-owner retirement\n"); + hooks.ReleaseRetirement(); + retire.join(); + OS::WaitTimerSetTestHooks(nullptr); + return 2; + } + + CallbackState callbackState; + OS::WaitTimer newTimer; + std::atomic initialized{ false }; + std::thread initialize([&newTimer, &callbackState, &initialized]() + { + initialized.store(SUCCEEDED(newTimer.Initialize(&callbackState, TimerCallback)), std::memory_order_release); + }); + + hooks.ReleaseRetirement(); + retire.join(); + initialize.join(); + OS::WaitTimerSetTestHooks(nullptr); + + if (!initialized.load(std::memory_order_acquire)) + { + std::printf("[waittimer-queue-retirement] FAILED: concurrent Initialize did not succeed\n"); + return 2; + } + + newTimer.Start(newTimer.GetDueTime(1)); + std::unique_lock lock(callbackState.Mutex); + if (!callbackState.Changed.wait_for(lock, 5s, [&callbackState]() noexcept { return callbackState.Ran; })) + { + std::printf("[waittimer-queue-retirement] FAILED: newly initialized timer did not dispatch\n"); + return 1; + } + + newTimer.Terminate(); + std::printf("[waittimer-queue-retirement] PASSED: concurrent initialization received a live timer queue\n"); + return 0; +} diff --git a/Tests/StandaloneTests/WaitTimerQueueTeardownRepro.cpp b/Tests/StandaloneTests/WaitTimerQueueTeardownRepro.cpp new file mode 100644 index 00000000..1c1563aa --- /dev/null +++ b/Tests/StandaloneTests/WaitTimerQueueTeardownRepro.cpp @@ -0,0 +1,147 @@ +// Copyright (c) Microsoft Corporation +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// Deterministic regression for the STL WaitTimer worker-pop teardown race. +// This target compiles Source/Task/WaitTimer_stl.cpp directly, rather than +// attempting to drive the race through public XTaskQueue APIs. The public API +// cannot reliably stop the worker after it has copied its internal timer +// pointer and released TimerQueue's lock. + +#include +#include "../../Source/Task/WaitTimer.h" + +#include +#include +#include +#include +#include +#include + +namespace +{ + using namespace std::chrono_literals; + + class QueueTeardownHooks final : public OS::WaitTimerTestHooks + { + public: + bool BeforeTimerInvoke() noexcept override + { + std::unique_lock lock(m_mutex); + m_timerPopped = true; + m_changed.notify_all(); + m_changed.wait(lock, [this]() noexcept { return m_releaseWorker; }); + return false; + } + + void WaitTimerImplDestructionStarted() noexcept override + { + std::lock_guard lock(m_mutex); + m_destructionStarted = true; + m_changed.notify_all(); + } + + bool WaitForTimerPop() + { + std::unique_lock lock(m_mutex); + return m_changed.wait_for(lock, 5s, [this]() noexcept { return m_timerPopped; }); + } + + bool DestructionStartedWithin(std::chrono::milliseconds timeout) + { + std::unique_lock lock(m_mutex); + return m_changed.wait_for(lock, timeout, [this]() noexcept { return m_destructionStarted; }); + } + + bool WaitForDestruction() + { + return DestructionStartedWithin(5s); + } + + void ReleaseWorker() + { + std::lock_guard lock(m_mutex); + m_releaseWorker = true; + m_changed.notify_all(); + } + + private: + std::mutex m_mutex; + std::condition_variable m_changed; + bool m_timerPopped = false; + bool m_destructionStarted = false; + bool m_releaseWorker = false; + }; +} + +int main() +{ + QueueTeardownHooks hooks; + OS::WaitTimerSetTestHooks(&hooks); + + OS::WaitTimer timer; + HRESULT hr = timer.Initialize(nullptr, [](void*) {}); + if (FAILED(hr)) + { + std::printf("[waittimer-queue-teardown] FAILED: Initialize returned 0x%08x\n", static_cast(hr)); + OS::WaitTimerSetTestHooks(nullptr); + return 2; + } + + // Keep the shared TimerQueue alive while the target timer is destroyed. + // Otherwise the old implementation synchronously joins the paused worker + // when retiring the final queue owner, obscuring the dispatch race. + OS::WaitTimer queueOwner; + hr = queueOwner.Initialize(nullptr, [](void*) {}); + if (FAILED(hr)) + { + std::printf("[waittimer-queue-teardown] FAILED: keepalive Initialize returned 0x%08x\n", static_cast(hr)); + timer.Terminate(); + OS::WaitTimerSetTestHooks(nullptr); + return 2; + } + + timer.Start(timer.GetDueTime(1)); + if (!hooks.WaitForTimerPop()) + { + std::printf("[waittimer-queue-teardown] FAILED: timer worker did not pop the due entry\n"); + timer.Terminate(); + queueOwner.Terminate(); + OS::WaitTimerSetTestHooks(nullptr); + return 2; + } + + std::atomic terminateComplete{ false }; + std::thread terminate([&]() + { + timer.Terminate(); + terminateComplete.store(true, std::memory_order_release); + }); + + const bool destructionStartedWhileWorkerBlocked = hooks.DestructionStartedWithin(100ms); + hooks.ReleaseWorker(); + terminate.join(); + const bool destructionStarted = hooks.WaitForDestruction(); + queueOwner.Terminate(); + OS::WaitTimerSetTestHooks(nullptr); + + if (!destructionStarted) + { + std::printf("[waittimer-queue-teardown] FAILED: WaitTimer implementation was not destroyed\n"); + return 2; + } + + if (destructionStartedWhileWorkerBlocked) + { + std::printf("[waittimer-queue-teardown] UNSAFE: implementation destruction began after worker copied its raw timer pointer\n"); + return 1; + } + + if (!terminateComplete.load(std::memory_order_acquire)) + { + std::printf("[waittimer-queue-teardown] FAILED: Terminate did not complete\n"); + return 2; + } + + std::printf("[waittimer-queue-teardown] PASSED: dispatch lifetime outlived worker teardown\n"); + return 0; +} \ No newline at end of file diff --git a/Tests/StandaloneTests/WaitTimerTestMemory.cpp b/Tests/StandaloneTests/WaitTimerTestMemory.cpp new file mode 100644 index 00000000..a69bb6c6 --- /dev/null +++ b/Tests/StandaloneTests/WaitTimerTestMemory.cpp @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// Standalone white-box WaitTimer tests compile the private implementation into +// their executable. Its allocator helpers are deliberately hidden from the +// shared-library ABI, so these tests provide the default allocator locally. + +#include "../../Source/Common/pch.h" +#include "../../Source/Global/mem.h" + +#include + +NAMESPACE_XBOX_HTTP_CLIENT_BEGIN + +void* http_memory::mem_alloc(size_t size) +{ + return std::malloc(size); +} + +void http_memory::mem_free(void* address) +{ + std::free(address); +} + +NAMESPACE_XBOX_HTTP_CLIENT_END \ No newline at end of file diff --git a/Tests/StandaloneTests/WaitTimerWrapperTerminateRepro.cpp b/Tests/StandaloneTests/WaitTimerWrapperTerminateRepro.cpp new file mode 100644 index 00000000..c7b6f0d5 --- /dev/null +++ b/Tests/StandaloneTests/WaitTimerWrapperTerminateRepro.cpp @@ -0,0 +1,161 @@ +// Copyright (c) Microsoft Corporation +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// Deterministic WaitTimer wrapper lifetime regression. This target compiles +// Source/Task/WaitTimer_stl.cpp directly so it can pause exactly after Start +// or Cancel reads the private m_impl pointer. Public XTaskQueue APIs and their +// hooks cannot expose or reliably schedule this wrapper-only interleaving. + +#include +#include "../../Source/Task/WaitTimer.h" + +#include +#include +#include +#include +#include + +namespace +{ + using namespace std::chrono_literals; + + class WrapperHooks final : public OS::WaitTimerTestHooks + { + public: + bool WaitTimerOperationLoaded(bool isStart) noexcept override + { + std::unique_lock lock(m_mutex); + m_operationLoaded = true; + m_isStart = isStart; + m_changed.notify_all(); + m_changed.wait(lock, [this]() noexcept { return m_releaseOperation; }); + m_operationResumedAfterDestruction = m_destructionStarted; + m_changed.notify_all(); + + // Avoid dereferencing the stale pointer after proving the race. + return !m_operationResumedAfterDestruction; + } + + void WaitTimerImplDestructionStarted() noexcept override + { + std::lock_guard lock(m_mutex); + m_destructionStarted = true; + m_changed.notify_all(); + } + + bool WaitForOperation(bool isStart) + { + std::unique_lock lock(m_mutex); + return m_changed.wait_for(lock, 5s, [this, isStart]() noexcept + { + return m_operationLoaded && m_isStart == isStart; + }); + } + + bool DestructionStartedWithin(std::chrono::milliseconds timeout) + { + std::unique_lock lock(m_mutex); + return m_changed.wait_for(lock, timeout, [this]() noexcept { return m_destructionStarted; }); + } + + bool WaitForDestruction() + { + return DestructionStartedWithin(5s); + } + + void ReleaseOperation() + { + std::lock_guard lock(m_mutex); + m_releaseOperation = true; + m_changed.notify_all(); + } + + bool OperationResumedAfterDestruction() + { + std::lock_guard lock(m_mutex); + return m_operationResumedAfterDestruction; + } + + private: + std::mutex m_mutex; + std::condition_variable m_changed; + bool m_operationLoaded = false; + bool m_isStart = false; + bool m_releaseOperation = false; + bool m_destructionStarted = false; + bool m_operationResumedAfterDestruction = false; + }; + + bool RunCase(bool isStart) + { + WrapperHooks hooks; + OS::WaitTimerSetTestHooks(&hooks); + + OS::WaitTimer timer; + HRESULT hr = timer.Initialize(nullptr, [](void*) {}); + if (FAILED(hr)) + { + std::printf("[waittimer-wrapper-terminate] FAILED: Initialize returned 0x%08x\n", static_cast(hr)); + OS::WaitTimerSetTestHooks(nullptr); + return false; + } + + std::thread operation([&timer, isStart]() + { + if (isStart) + { + timer.Start(timer.GetDueTime(60000)); + } + else + { + timer.Cancel(); + } + }); + + if (!hooks.WaitForOperation(isStart)) + { + std::printf("[waittimer-wrapper-terminate] FAILED: %s did not observe m_impl\n", isStart ? "Start" : "Cancel"); + hooks.ReleaseOperation(); + operation.join(); + timer.Terminate(); + OS::WaitTimerSetTestHooks(nullptr); + return false; + } + + std::thread terminate([&timer]() { timer.Terminate(); }); + const bool destructionStartedWhileOperationBlocked = hooks.DestructionStartedWithin(100ms); + hooks.ReleaseOperation(); + operation.join(); + terminate.join(); + const bool unsafe = hooks.OperationResumedAfterDestruction(); + const bool destructionStarted = hooks.WaitForDestruction(); + OS::WaitTimerSetTestHooks(nullptr); + + if (destructionStartedWhileOperationBlocked || unsafe) + { + std::printf("[waittimer-wrapper-terminate] UNSAFE: implementation destruction began while %s held its raw implementation reference\n", isStart ? "Start" : "Cancel"); + return false; + } + + if (!destructionStarted) + { + std::printf("[waittimer-wrapper-terminate] FAILED: implementation destruction did not run after %s completed\n", isStart ? "Start" : "Cancel"); + return false; + } + + return true; + } +} + +int main() +{ + const bool startPassed = RunCase(true); + const bool cancelPassed = RunCase(false); + if (!startPassed || !cancelPassed) + { + return 1; + } + + std::printf("[waittimer-wrapper-terminate] PASSED: Start and Cancel cannot race implementation teardown\n"); + return 0; +} \ No newline at end of file