Skip to content

Fix WaitTimer teardown and wrapper lifetime races#1005

Open
jhugard wants to merge 4 commits into
mainfrom
user/jhugard/fix-stl-waittimer-teardown
Open

Fix WaitTimer teardown and wrapper lifetime races#1005
jhugard wants to merge 4 commits into
mainfrom
user/jhugard/fix-stl-waittimer-teardown

Conversation

@jhugard

@jhugard jhugard commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Fix WaitTimer teardown and wrapper lifetime races

Summary

This PR fixes two independent timer lifetime races:

  1. The STL backend can dispatch a due delayed timer after queue-owned teardown has begun.
  2. WaitTimer::Start() or Cancel() can overlap WaitTimer::Terminate() and use an implementation that termination has already destroyed.

The change preserves the pointer-keyed cancellation and unconditional wake behavior restored by PR #998. That behavior is required to avoid reintroducing the delayed-callback strand that #998 corrected. The TaskQueue timer-arming improvements from PR #975 and its follow-ups are unchanged.

Related reports:

Changes

STL timer dispatch lifetime

The STL queue now keeps each timer's callback payload and dispatch bookkeeping in a
shared WaitTimerState. A due queue entry holds a shared_ptr lease on that
state, so a dispatch that is already under way owns the state it needs and can
never be invalidated by concurrent teardown of the raw WaitTimerImpl.

Teardown prevents new dispatches (BeginTerminate), removes matching
pointer-keyed entries from the queue, and then waits for an already-started
dispatch to quiesce before the state is released.

Queue ownership is shared. The single process-wide TimerQueue is held by a
shared_ptr from every live timer and, critically, by the worker thread itself
(captured in Init). A separate m_timerCount, guarded by g_timerQueueMutex,
lets the last timer retire the queue; the decrement and the last-owner decision
run under the same lock that Initialize takes to adopt a timer, so a retiring
queue can never be resurrected by a concurrent Initialize. Because the worker
holds its own reference, a callback may tear down its own timer (and the last
queue reference): the teardown skips the quiesce wait when it runs on the worker
thread, and ~TimerQueue detaches instead of self-joining, so callback-driven
shutdown neither deadlocks nor touches freed state.

The existing pointer-keyed cancellation scan remains in place. Every Set()
continues to wake the worker unconditionally. There is no generation-based
stale-entry filter, so a concurrent re-arm cannot discard an otherwise due
callback as the reverted #975 backend did.

Shared WaitTimer wrapper lifetime

The shared WaitTimer wrapper now serializes Initialize(), Start(), Cancel(), and the ownership transfer in Terminate().

Terminate() removes the implementation from the wrapper while holding the wrapper lock, then performs the potentially blocking backend teardown after releasing that lock. The blocking quiesce never runs under the wrapper lock, so a timer callback that re-enters Start(), Cancel(), or its own teardown cannot deadlock against it.

The synchronization applies to STL, Win32, and iOS implementations because the ownership race is in the shared wrapper contract rather than any one backend timer primitive.

Commit structure

The production changes are layered as two red/green pairs for review:

  1. Add deterministic STL WaitTimer teardown repro
  2. Fix STL WaitTimer queue teardown lifetime
  3. Add WaitTimer wrapper termination repro
  4. Serialize WaitTimer wrapper termination

Each repro commit fails before its corresponding fix commit, verified on Linux:
waittimer-queue-teardown-linux fails on commit 1 and passes on commit 2, and
waittimer-wrapper-terminate-linux fails on commit 3 and passes on commit 4.
The first pair demonstrates queue-owned teardown overlapping a due STL timer
dispatch; the second demonstrates Start() and Cancel() overlapping
Terminate() after observing the timer implementation.

The teardown fix also adds a waittimer-queue-retirement regression. This is a
regression guard, not a third red/green pair: the shared-queue design is
introduced in that same commit already-correct — the last-owner refcount
decrement and the queue reset both run under g_timerQueueMutex, so a retiring
queue cannot be resurrected by a concurrent Initialize() — so the test has no
red predecessor in this branch. It exists to lock that ordering in against
future regressions.

Testing approach

Public-API coverage is preferred, and the existing TaskQueue regressions
(taskqueue-starvation, taskqueue-poll-strand) drive the timer through
XTaskQueue. The three new WaitTimer regressions, however, target races that
have no reliable public-API interleaving:

  • The teardown race requires pausing the STL worker in the exact window after it
    copies a due entry and releases the queue lock. No public XTaskQueue
    operation can stop the worker at that point.
  • The wrapper race requires pausing Start()/Cancel() immediately after they
    read the private m_impl pointer while Terminate() runs concurrently. That
    interleaving is internal to WaitTimer, and public queue references keep the
    port alive while scheduling is in progress, so it cannot be driven from the
    public API.
  • The retirement race requires pausing inside last-owner retirement while a
    concurrent Initialize() runs.

These three tests are therefore built as white-box standalone executables that
compile Source/Task/WaitTimer_stl.cpp directly and drive it through an
internal WaitTimerTestHooks seam. The seam is source-internal and is not part
of the public libHttpClient ABI; when no hooks are installed, production pays
only a single relaxed atomic load. This keeps the deterministic timer-lifetime
coverage beside the timer implementation, which is internally consistent for
this branch.

Two limitations of this coverage are worth calling out for review:

  • Each repro pauses the timer under test with a hook, then infers that the
    competing Terminate() or Initialize() thread reached the contended region
    from a bounded wait rather than from a second explicit "contender arrived"
    synchronization point. On the fixed revisions the contended path blocks
    deterministically, so the observed result is correct; the residual exposure is
    a scheduling-dependent false negative on the pre-fix revisions under heavy
    load. A follow-up could add that second synchronization point.
  • The wrapper serialization is applied identically to the STL, Win32, and iOS
    backends, but only the shared contract is executed on Linux. The Win32 and
    iOS paths receive the same mechanical m_mutex change and are not covered by
    a native regression test.

Validation

Built on Linux (Release, STL backend) and validated at each commit boundary: the
two repro commits fail before their fixes and pass after them. On the final
revision, the full taskqueue label passes:

ctest --test-dir <build> --output-on-failure -L taskqueue

Passed:

taskqueue-starvation-linux
taskqueue-poll-strand-linux
waittimer-queue-teardown-linux
waittimer-queue-retirement-linux
waittimer-wrapper-terminate-linux

The existing poll-strand regression continues to pass, confirming the change preserves the #998 scheduling behavior while adding lifetime protection.

Windows x64 validation built libHttpClient.UnitTest.TAEF in both Debug and
Release configurations, including the updated WaitTimer_win32.cpp, then ran
the full TAEF suite in process:

TE.exe libHttpClient.UnitTest.TAEF.dll /inproc

Both runs passed all 103 tests (Total=103, Passed=103, Failed=0). This
validates that the Win32 wrapper change does not regress the existing Windows
unit-test suite; however, it does not cover the missing native deterministic wrapper
race repro described above.

Scope

  • No public XTaskQueue or HTTP client API changes.
  • The new WaitTimer regressions are white-box standalone tests that compile the
    private STL timer implementation directly; the WaitTimerTestHooks seam is
    source-internal and is not exported from the shared library.
  • No changes to TaskQueue timer-arming behavior.
  • No changes to the delayed-callback cancellation contract.
  • The Linux-specific queue dispatch hardening is complemented by wrapper lifetime synchronization across STL, Win32, and iOS backends.

Review focus

  • Dispatch gating and quiescing order during STL timer teardown.
  • Preservation of pointer-keyed cancellation and unconditional worker wake behavior.
  • Worker thread and global queue lifetime during last-timer retirement.
  • Wrapper-lock scope: ownership transfer is serialized, while backend quiescing occurs outside the wrapper lock.
  • The red-to-green layering of both regression pairs.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens WaitTimer lifetime behavior to eliminate two race classes: (1) STL timer dispatch occurring after queue-owned teardown begins, and (2) wrapper-level races where Start()/Cancel() can overlap Terminate() and use a destroyed implementation. It does this while preserving the pointer-keyed cancellation and unconditional worker wake behavior needed to avoid the previously observed delayed-callback strand.

Changes:

  • STL backend: move callback payload/dispatch bookkeeping into a shared WaitTimerState leaseable by a due queue entry, and gate/quiesce dispatch during teardown while keeping pointer-keyed cancellation.
  • Wrapper contract: serialize Initialize(), Start(), Cancel(), and the impl ownership transfer in Terminate() (STL/Win32/iOS), ensuring backend teardown happens outside the wrapper lock.
  • Add deterministic white-box standalone Linux regressions (queue teardown, queue retirement ordering, wrapper termination) and wire them into Linux CTest.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Tests/StandaloneTests/WaitTimerWrapperTerminateRepro.cpp Deterministic repro/guard for wrapper Start()/Cancel() overlapping Terminate() using internal test hooks.
Tests/StandaloneTests/WaitTimerTestMemory.cpp Provides local default allocator plumbing for white-box standalone tests that compile private implementation.
Tests/StandaloneTests/WaitTimerQueueTeardownRepro.cpp Deterministic repro/guard for STL worker-pop vs teardown dispatch lifetime race.
Tests/StandaloneTests/WaitTimerQueueRetirementRepro.cpp Deterministic guard for last-owner queue retirement not being resurrectable by concurrent Initialize().
Tests/StandaloneTests/TaskQueueStarvationRepro.cpp Relocated/packaged Linux standalone starvation repro to the StandaloneTests folder.
Tests/StandaloneTests/SingleQueuePollStrandRepro.cpp Relocated/packaged Linux standalone poll-strand repro to the StandaloneTests folder.
Source/Task/WaitTimer.h Adds internal test hook seam and wrapper mutex to serialize impl lifetime transitions.
Source/Task/WaitTimer_win32.cpp Serializes wrapper operations with the new mutex to prevent impl use-after-free races.
Source/Task/WaitTimer_stl.cpp Implements shared-state dispatch leasing + teardown gating/quiescing; adds internal hook plumbing and shared queue lifetime management.
Source/Task/iOS/ios_WaitTimer.mm Serializes wrapper operations with the new mutex to prevent impl lifetime races.
Build/libHttpClient.Linux/CMakeLists.txt Adds new Linux CTest standalone targets for deterministic WaitTimer lifetime regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants