Skip to content

Fix timer lifetime and C++ callback bookkeeping after reset - #910

Open
radkesvat wants to merge 7 commits into
ithewei:masterfrom
radkesvat:fix/timer-reset-lifetime
Open

radkesvat wants to merge 7 commits into
ithewei:masterfrom
radkesvat:fix/timer-reset-lifetime

Conversation

@radkesvat

Copy link
Copy Markdown
Contributor

Summary

This PR fixes three timer lifetime and bookkeeping bugs:

  • Resetting and deleting a native timer can free storage that pending dispatch still references.

  • Resetting a pending finite timer can leave the C++ wrapper holding a stale native pointer.

  • Completion of an old C++ callback can remove a replacement created with the same timer ID.

The production changes are small; most of the diff adds focused C and C++ regression tests.

Changes

1. Preserve native timer lifetime across reset and deletion

hloop_process_pendings() retains the current event and caches its pending_next pointer before invoking application code. The pending flag must therefore remain set while dispatch still references the allocation.

Previously, htimer_reset() called EVENT_RESET(), which cleared pending without removing those references. A subsequent htimer_del() could free the timer immediately, leaving dispatch to access freed storage. This affects both a timer that resets and deletes itself and another pending timer reset and deleted by an earlier callback, including the next timer already cached in the same priority queue. The application does not need to access the timer after deleting it.

Reset now preserves pending and marks a pending expiration as superseded using a timer-specific pending_reset flag. Dispatch skips the superseded callback, clears both flags when it finishes processing that pending entry, and then performs any deferred destruction. A retained timer keeps its replacement schedule.

The two flags separate allocation lifetime from callback delivery: preserving storage must not deliver the obsolete expiration. htimer_reset() is the only current caller of EVENT_RESET(); I/O and idle callback handling are unchanged.

2. Synchronize the C++ repeat count after reset

The native timer consumes a finite repetition when an expiration becomes pending. EventLoop::onTimer() decrements its separate wrapper count only when the callback is delivered. Reset can suppress that delivery and leave the counts different, causing the wrapper to retain a native pointer after the native timer completes.

This adds the exported accessor:

uint32_t htimer_repeat(const htimer_t* timer);

EventLoop::resetTimer() reads it immediately after htimer_reset(), inside the existing owner-loop operation, and copies the result into the wrapper.

Reading after reset is necessary for self-rearming: a final callback observes a native count of zero, but resetting that live timeout revives the count to one. The wrapper must observe the revived count to retain the rearmed timer. Its existing finite-count decrement before invoking application code remains in place.

The accessor documents the live-handle and owner-loop/thread requirements and provides no additional pointer validation or synchronization. It reports remaining native expirations, including INFINITE. A collected expiration still consumes a repetition even if reset suppresses its callback; existing counting and zero-to-one revival semantics are preserved.

<details> <summary>Example: finite-count divergence before the fix</summary>

For a timer initially configured with two repetitions:

Step | Native remaining count | C++ remaining count -- | -- | -- Timer created | 2 | 2 First expiration becomes pending | 1 | 2 Another callback resets it; old delivery is suppressed | 1 | 2 Replacement expiration is delivered | 0 | 1

</details>

## Summary

This PR fixes three timer lifetime and bookkeeping bugs:

  • Resetting and deleting a native timer can free storage that pending dispatch still references.
  • Resetting a pending finite timer can leave the C++ wrapper holding a stale native pointer.
  • Completion of an old C++ callback can remove a replacement created with the same timer ID.

The production changes are small; most of the diff adds focused C and C++ regression tests.

Changes

1. Preserve native timer lifetime across reset and deletion

hloop_process_pendings() retains the current event and caches its pending_next pointer before invoking application code. The pending flag must therefore remain set while dispatch still references the allocation.

Previously, htimer_reset() called EVENT_RESET(), which cleared pending without removing those references. A subsequent htimer_del() could free the timer immediately, leaving dispatch to access freed storage. This affects both a timer that resets and deletes itself and another pending timer reset and deleted by an earlier callback, including the next timer already cached in the same priority queue. The application does not need to access the timer after deleting it.

Reset now preserves pending and marks a pending expiration as superseded using a timer-specific pending_reset flag. Dispatch skips the superseded callback, clears both flags when it finishes processing that pending entry, and then performs any deferred destruction. A retained timer keeps its replacement schedule.

The two flags separate allocation lifetime from callback delivery: preserving storage must not deliver the obsolete expiration. htimer_reset() is the only current caller of EVENT_RESET(); I/O and idle callback handling are unchanged.

2. Synchronize the C++ repeat count after reset

The native timer consumes a finite repetition when an expiration becomes pending. EventLoop::onTimer() decrements its separate wrapper count only when the callback is delivered. Reset can suppress that delivery and leave the counts different, causing the wrapper to retain a native pointer after the native timer completes.

This adds the exported accessor:

uint32_t htimer_repeat(const htimer_t* timer);

EventLoop::resetTimer() reads it immediately after htimer_reset(), inside the existing owner-loop operation, and copies the result into the wrapper.

Reading after reset is necessary for self-rearming: a final callback observes a native count of zero, but resetting that live timeout revives the count to one. The wrapper must observe the revived count to retain the rearmed timer. Its existing finite-count decrement before invoking application code remains in place.

The accessor documents the live-handle and owner-loop/thread requirements and provides no additional pointer validation or synchronization. It reports remaining native expirations, including INFINITE. A collected expiration still consumes a repetition even if reset suppresses its callback; existing counting and zero-to-one revival semantics are preserved.

Example: finite-count divergence before the fix

For a timer initially configured with two repetitions:

Step Native remaining count C++ remaining count
Timer created 2 2
First expiration becomes pending 1 2
Another callback resets it; old delivery is suppressed 1 2
Replacement expiration is delivered 0 1

The native timer then completes and is freed, while the wrapper retains its record. A later resetTimer(id) or killTimer(id) can access the freed native pointer.

With the fix, reset synchronizes the wrapper count to one. The replacement expiration then brings both counts to zero, allowing both records to retire. Preserving native pending storage alone would not correct this wrapper bookkeeping.

3. Preserve a replacement created during the old callback

A completing callback can call killTimer(id) and then setTimer(..., id) to create a replacement with the same explicit ID. Previously, completion erased the ID whenever the captured old timer's count reached zero. That removed the replacement's wrapper record while its native timer remained scheduled, losing its callback and ID-based cancellation path.

Completion now performs a fresh lookup and erases the entry only if it still refers to the captured old TimerPtr. The existing retained shared pointer keeps the old record alive across application code, and the fresh lookup avoids reusing an iterator that the callback may have invalidated.

Normal completion removes the old record, successful self-rearming retains it, self-cancellation tolerates an absent entry, and a different replacement remains callable and cancellable.

Scope: this covers cancelling a timer before recreating it with the same ID. Overwriting an ID that still belongs to an active timer remains a separate issue; this PR does not make that case safe or choose reject-versus-replace behavior.

Regression tests

unittest/timer_reset_test.c adds 12 native cases covering scheduled reset, pending-expiration suppression, self-rearming, reset followed by deletion of the executing or another pending timer, same- and different-priority dispatch, repeated resets with keep/cancel outcomes, finite repetition, and replacement allocation during callbacks. Assertions check callback behavior, timer and active-event counts, allocation balance, and preservation of unrelated timers.

unittest/eventloop_timer_test.cpp adds eight C++ cases covering finite-count reconciliation, repeated resets, reset between delivered occurrences, one-shot self-rearming, infinite timers, and same-ID replacement delivery and cancellation, with self-cancellation and normal-completion controls. Callback-captured ownership probes observed through weak_ptr verify that wrapper records actually retire; callback counts alone would miss stale entries.

Both suites use checks that remain active under NDEBUG. Test helpers move selected live timer deadlines near the current clock while maintaining heap ordering, then use real dispatch without long sleeps or external services. The native scheduled-reset case checks the exact cached-clock deadline, including existing 100-ms rounding, before forcing delivery.

The C++ fixture drives the real EventLoop::run() with HLOOP_FLAG_RUN_ONCE, including queued operations, and destroys the wrapper before freeing its external loop.

Both suites are included in Make, CMake, and scripts/unittest.sh; the shell runner propagates their failures. The C++ target is conditional on WITH_EVPP. CMake groups the DNS and timer wrapper tests under EVPP_UNITTEST_TARGETS while retaining existing dependencies, including the Windows event test.

Validation

Local Linux validation of the functional changes:

  • Both timer suites passed with CMake Debug and optimized Release builds.
  • Both passed with AddressSanitizer and UndefinedBehaviorSanitizer, with the library and tests instrumented and leak detection enabled.
  • A clean Make unittest build succeeded, followed by successful execution of both timer suites. These were focused runtime checks of the two timer executables.
  • With WITH_EVPP=OFF and WITH_HTTP=OFF, the native timer suite built and passed, and the C++ timer target was omitted.
  • Changed-range source formatting, complete test formatting, and git diff --check passed.

After the final comment and CMake list-name cleanup in 53ffe3d, CMake configuration and generated unittest dependencies were checked with EVPP enabled and disabled. Both wrapper targets are included only when EVPP is enabled, and the native timer target is retained in both configurations.

The [Linux CI job at [c298d31](https://github.com/radkesvat/libhv/actions/runs/39508569837/job/118923600435)](https://github.com/radkesvat/libhv/actions/runs/39508569837/job/118923600435) also passed both new timer suites. The [CI run for the latest commit, [53ffe3d](https://github.com/radkesvat/libhv/actions/runs/39511164860)](https://github.com/radkesvat/libhv/actions/runs/39511164860), is linked separately.

Execution of these timer suites on Windows/MSVC and macOS is not covered by the validation above.

Focused Linux CMake commands

From the libhv checkout:

cmake -S . -B build/timer-debug -DCMAKE_BUILD_TYPE=Debug \
  -DBUILD_UNITTEST=ON -DBUILD_EXAMPLES=OFF \
  -DBUILD_STATIC=ON -DBUILD_SHARED=OFF -DWITH_EVPP=ON

cmake --build build/timer-debug \
  --target timer_reset_test eventloop_timer_test -j8

timeout 60s build/timer-debug/bin/timer_reset_test
timeout 60s build/timer-debug/bin/eventloop_timer_test

For Release, use a separate build directory and CMAKE_BUILD_TYPE=Release.

For the sanitizer run, use a separate Debug directory and add:

  • -fsanitize=address,undefined -fno-omit-frame-pointer to both CMAKE_C_FLAGS and CMAKE_CXX_FLAGS.
  • -fsanitize=address,undefined to CMAKE_EXE_LINKER_FLAGS.

Run each executable with ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 and UBSAN_OPTIONS=halt_on_error=1.

Suggested commit review order
Commit Review focus
d51399d Native pending lifetime and obsolete-expiration suppression
e207033 Native regression cases and test registration
a80e2bc Exact reset-deadline assertion
8e31749 Native repeat accessor, C++ synchronization, and corresponding tests
27fa24c Replacement identity during callback cleanup and corresponding tests
c298d31 Public accessor contract documentation
53ffe3d Accurate forced-deadline comments and generic EVPP test grouping

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.

1 participant