Repository navigation
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 itspending_nextpointer before invoking application code. Thependingflag must therefore remain set while dispatch still references the allocation.Previously,
htimer_reset()calledEVENT_RESET(), which clearedpendingwithout removing those references. A subsequenthtimer_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
pendingand marks a pending expiration as superseded using a timer-specificpending_resetflag. 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 ofEVENT_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:
EventLoop::resetTimer()reads it immediately afterhtimer_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>
## SummaryThis PR fixes three timer lifetime and bookkeeping bugs:
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 itspending_nextpointer before invoking application code. Thependingflag must therefore remain set while dispatch still references the allocation.Previously,
htimer_reset()calledEVENT_RESET(), which clearedpendingwithout removing those references. A subsequenthtimer_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
pendingand marks a pending expiration as superseded using a timer-specificpending_resetflag. 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 ofEVENT_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:
EventLoop::resetTimer()reads it immediately afterhtimer_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:
The native timer then completes and is freed, while the wrapper retains its record. A later
resetTimer(id)orkillTimer(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 thensetTimer(..., 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.cadds 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.cppadds 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 throughweak_ptrverify 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()withHLOOP_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 onWITH_EVPP. CMake groups the DNS and timer wrapper tests underEVPP_UNITTEST_TARGETSwhile retaining existing dependencies, including the Windows event test.Validation
Local Linux validation of the functional changes:
unittestbuild succeeded, followed by successful execution of both timer suites. These were focused runtime checks of the two timer executables.WITH_EVPP=OFFandWITH_HTTP=OFF, the native timer suite built and passed, and the C++ timer target was omitted.git diff --checkpassed.After the final comment and CMake list-name cleanup in
53ffe3d, CMake configuration and generatedunittestdependencies 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_testFor 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-pointerto bothCMAKE_C_FLAGSandCMAKE_CXX_FLAGS.-fsanitize=address,undefinedtoCMAKE_EXE_LINKER_FLAGS.Run each executable with
ASAN_OPTIONS=detect_leaks=1:halt_on_error=1andUBSAN_OPTIONS=halt_on_error=1.Suggested commit review order
d51399de207033a80e2bc8e3174927fa24cc298d3153ffe3d