-
-
Notifications
You must be signed in to change notification settings - Fork 142
fix: Resolve timer bugs causing lost timers and resource leaks #1894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Resolve timer bugs causing lost timers and resource leaks #1894
Conversation
Fixed five bugs in Timers.cpp: - Inverted EAGAIN retry condition causing timer loss when pipe buffer full - File descriptor leak (fd_[1] never closed in Destroy) - deletedTimers_ accumulation for immediate timers cleared before callback - Data race on stopped flag (now atomic) - Non-EAGAIN write errors incorrectly treated as success Additional improvements: - Clean up deletedTimers_ when timer cancelled during EAGAIN retry - Guard isBufferFull reset to only occur on actual write success
edusperoni
commented
Dec 2, 2025
@adrian-niculescu thanks for the fixes!
Unfortunately, I think we'll do a full refactor of timers soon. When I initally wrote it I thought that the NDK looper was similar to the Java looper. Turns out they're completely different, which leads to breaking hevavior. Example:
newTimeout(() => console.log(1), 0);
oldTimeout(() => console.log(2), 0);
for(let i =0; i< 100; i++) newTimeout(() => console.log('loop', i), 0);
oldTimeout(() => console.log(3), 10);
You'd expect it to run something like:
1, 2, loop 0-99, 3
but you'll often get things like:
2, 1, loop 0-30, 3, loop 31-99
which means that the ordering is wrong and you'll get some Java code executing before the timeouts. In fact, if you replace setTimeout with the new timeouts, you gets tons of issues even with the example apps. So the new idea is to implement as a mix of java and C++ (doing the most to avoid crossing the JNI bridge unecessarily)
@adrian-niculescu thanks for the fixes!
Unfortunately, I think we'll do a full refactor of timers soon. When I initally wrote it I thought that the NDK looper was similar to the Java looper. Turns out they're completely different, which leads to breaking hevavior. Example:
newTimeout(() => console.log(1), 0); oldTimeout(() => console.log(2), 0); for(let i =0; i< 100; i++) newTimeout(() => console.log('loop', i), 0); oldTimeout(() => console.log(3), 10);You'd expect it to run something like: 1, 2, loop 0-99, 3 but you'll often get things like: 2, 1, loop 0-30, 3, loop 31-99
which means that the ordering is wrong and you'll get some Java code executing before the timeouts. In fact, if you replace setTimeout with the new timeouts, you gets tons of issues even with the example apps. So the new idea is to implement as a mix of java and C++ (doing the most to avoid crossing the JNI bridge unecessarily)
Cool! Looking forward to it! I remember using the timers in tns-core-modules - they were basically straight NativeScript plugins, which worked fine for a long time until your more performant 2022-2023 implementation of these timers in the Android and iOS runtimes.
This PR fixes five bugs in
Timers.cppthat could cause timers to be lost, file descriptors to leak, and memory to accumulate.1. Inverted EAGAIN retry condition (line 162)
When the pipe buffer fills up and
write()returns EAGAIN, the retry loop had the wrong condition:This caused live timers to be silently dropped when the pipe was full, because the loop never executed for timers that weren't deleted.
Additionally, if a timer is cancelled while waiting for buffer space, the fix now cleans up
deletedTimers_to prevent leaking the timer ID:2. File descriptor leak (line 192)
Destroy()was closing only the read end of the pipe:Now both ends are properly closed:
3. deletedTimers_ accumulation (line 323)
Immediate timers (delay ≤ 0) skip the
sortedTimers_queue and write directly to the pipe. If cleared before the callback runs, their IDs were added todeletedTimers_but never removed.Added cleanup in
PumpTimerLoopCallback:4. Data race on stopped flag (Timers.h line 131)
The
stoppedflag was written under mutex inDestroy()but read without synchronization inPumpTimerLoopCallback(), causing undefined behavior per C++ standard.5. Non-EAGAIN write errors drop timers (line 92)
The condition
if (result != -1 || errno != EAGAIN)treated all non-EAGAIN errors (EPIPE, EBADF, EINTR) as success, disabling fallback scheduling:Additional fix: isBufferFull reset guard (line 171)
The
isBufferFulloptimization flag was being reset even whenwrite()failed with non-EAGAIN errors:This prevents incorrectly re-enabling the immediate timer optimization when the pipe is broken.