-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): never evict a scheduled timer's ref state (#10447) #10538
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| Fixed `timer.unref()` being forgotten after 65,536 later timers (#10447). | ||
|
|
||
| **What broke.** #6084 bounded the id→ref-state registry at 65,536 entries. It evicted by insertion order and never checked whether an id was still scheduled. A live, long-delay `unref()`'d timer was evicted once 65,536 newer timers had been created. The lookup then read the missing id as ref'd, so the process stayed alive until that timer fired and ran a callback the program had detached. rate-limiter-flexible, which creates one timer per key, lingered 12–15 s after a 1M-operation run. The evicted id also dropped out of `is_known_timer_id`, so `.hasRef()`, `.ref()`, `.unref()`, `.constructor` and `+t` stopped resolving on the handle. The id→kind table (`Timeout`/`Immediate`) had the same cap. Once full, it scanned all 65,536 keys for the minimum on every `setTimeout`, so 200k `clearTimeout(setTimeout(f, 1000))` cost 126 billion instructions. | ||
|
|
||
| **Fix.** `crates/perry-runtime/src/timer/ref_states.rs`: | ||
| - One registry now holds `has_ref`, `kind` and a `scheduled` flag per id. | ||
| - Scheduling returns a `ScheduledTimerId` token that lives in the queue entry itself (`CallbackTimer`, `IntervalTimer`, and the mock-timer entries). Dropping the entry retires the id. Every removal path drops the entry: firing, `clearTimeout`/`clearInterval`/`clearImmediate`, agent purge, and mock clear/reset. | ||
| - Only retired ids are eviction candidates, so the map holds at most live timers + 65,536 entries. | ||
| - Scheduling does one registry insert, and there is no per-insert scan. | ||
| - The whole-queue liveness scans (`ownership.rs`) take the registry lock once per scan instead of once per entry. | ||
| - The `timer.rs` filters test `allow_unref` before looking up `has_ref`. | ||
| - The registry reports a `timer.ref_states` row in `PERRY_GC_CENSUS`. | ||
| - The stale `TIMER_HANDLE_KINDS` entry is removed from `scripts/gc_runtime_root_holders.json`. | ||
|
|
||
| **Validation.** | ||
| - The gap test `test_gap_10447_timer_ref_state_eviction` matches Node byte for byte. On the baseline, every handle loses its methods and four unref'd callbacks fire. | ||
| - New unit tests in `ref_states.rs` cover: scheduled ids surviving churn, live timers beyond the cap, 1M set+clear cycles staying at 65,536 entries, and an end-to-end churn through the real queues. | ||
| - Gap suite: 814/820 pass; the 6 failures are the baseline's known ones. | ||
| - Instructions: 200k set+clear −99.3 %, 100k chained `setImmediate` −95.1 %. Below-cap schedule/fire/clear workloads are 4–8 % faster, and `asyncpipe` is flat (+0.1 %). | ||
| - Registry size is 65,536 entries after both 1M and 3M set+clear cycles. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,10 +12,9 @@ mod async_lifecycle; | |
| use crate::promise::{js_promise_new, js_promise_resolve, Promise}; | ||
| use async_lifecycle::{enqueue_destroy_ids, IntervalCallback}; | ||
| use std::any::Any; | ||
| use std::collections::HashMap; | ||
| use std::sync::{ | ||
| atomic::{AtomicBool, AtomicU64, Ordering}, | ||
| LazyLock, Mutex, | ||
| Mutex, | ||
| }; | ||
| use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; | ||
|
|
||
|
|
@@ -121,15 +120,6 @@ fn schedule_promise_timer(delay_ms: f64, value: f64, has_ref: bool) -> *mut Prom | |
| promise | ||
| } | ||
|
|
||
| fn timer_has_ref_state(id: i64) -> bool { | ||
| TIMER_REF_STATES | ||
| .lock() | ||
| .unwrap() | ||
| .as_ref() | ||
| .and_then(|s| s.states.get(&id).copied()) | ||
| .unwrap_or(true) | ||
| } | ||
|
|
||
| fn other_event_sources_keep_loop_alive() -> bool { | ||
| has_refed_callback_timer() | ||
| || has_refed_interval_timer() | ||
|
|
@@ -337,6 +327,8 @@ struct CallbackTimer { | |
| /// in. Only that agent — or a pump acting for it, e.g. Android's UI thread | ||
| /// for the primary agent — may fire it. | ||
| owner: crate::agent::AgentId, | ||
| /// #10447: pins `id`'s ref state while queued; dropping it retires the id. | ||
| _scheduled: ScheduledTimerId, | ||
| } | ||
|
|
||
| // SAFETY: the closure POINTER targets global compiled code, but the closure | ||
|
|
@@ -353,7 +345,6 @@ pub const MOCK_TIMERS_ALL_APIS: u32 = MOCK_TIMERS_API_DATE | |
| | MOCK_TIMERS_API_SET_INTERVAL | ||
| | MOCK_TIMERS_API_SET_IMMEDIATE; | ||
|
|
||
| #[derive(Clone)] | ||
| struct MockCallbackTimer { | ||
| id: i64, | ||
| kind: CallbackTimerKind, | ||
|
|
@@ -362,11 +353,11 @@ struct MockCallbackTimer { | |
| args: Vec<f64>, | ||
| context: crate::async_context::AsyncContextSnapshot, | ||
| cleared: bool, | ||
| _scheduled: ScheduledTimerId, | ||
| } | ||
|
|
||
| unsafe impl Send for MockCallbackTimer {} | ||
|
|
||
| #[derive(Clone)] | ||
| struct MockIntervalTimer { | ||
| id: i64, | ||
| callback: i64, | ||
|
|
@@ -375,6 +366,7 @@ struct MockIntervalTimer { | |
| args: Vec<f64>, | ||
| context: crate::async_context::AsyncContextSnapshot, | ||
| cleared: bool, | ||
| _scheduled: ScheduledTimerId, | ||
| } | ||
|
|
||
| unsafe impl Send for MockIntervalTimer {} | ||
|
|
@@ -416,11 +408,11 @@ use ownership::{has_refed_callback_timer, has_refed_interval_timer, has_refed_pr | |
| pub(crate) use ownership::{purge_agent_timers, timer_phase_work_pending}; | ||
|
|
||
| pub(crate) use gc_scan::{new_timer_root_scan_state, scan_timer_roots_mut_step}; | ||
| use ref_states::{TimerRefStates, TIMER_REF_STATES_CAP}; | ||
| use ref_states::{ | ||
| register_scheduled_timer, set_timer_ref_state, timer_handle_kind, timer_has_ref_state, | ||
| ScheduledTimerId, | ||
| }; | ||
|
|
||
| static TIMER_REF_STATES: Mutex<Option<TimerRefStates>> = Mutex::new(None); | ||
| static TIMER_HANDLE_KINDS: LazyLock<Mutex<HashMap<i64, CallbackTimerKind>>> = | ||
| LazyLock::new(|| Mutex::new(HashMap::new())); | ||
| static WARNED_NEGATIVE_TIMER_DELAY: AtomicBool = AtomicBool::new(false); | ||
| static WARNED_NAN_TIMER_DELAY: AtomicBool = AtomicBool::new(false); | ||
|
|
||
|
|
@@ -607,29 +599,12 @@ fn normalize_timer_delay(delay_value: f64) -> u64 { | |
| } | ||
| } | ||
|
|
||
| fn set_timer_ref_state(id: i64, has_ref: bool) { | ||
| ref_states::TIMER_IDS_NONEMPTY.arm(); | ||
| let mut slot = TIMER_REF_STATES.lock().unwrap(); | ||
| slot.get_or_insert_with(TimerRefStates::default) | ||
| .insert_bounded(id, has_ref, TIMER_REF_STATES_CAP); | ||
| } | ||
|
|
||
| fn record_timer_handle_kind(id: i64, kind: CallbackTimerKind) { | ||
| let mut kinds = TIMER_HANDLE_KINDS.lock().unwrap(); | ||
| if kinds.len() >= TIMER_REF_STATES_CAP && !kinds.contains_key(&id) { | ||
| if let Some(oldest) = kinds.keys().copied().min() { | ||
| kinds.remove(&oldest); | ||
| } | ||
| } | ||
| kinds.insert(id, kind); | ||
| } | ||
|
|
||
| /// Synthetic constructor object for `Timeout`/`Immediate` native handles. | ||
| /// Timer ids outlive queue removal, so the kind table retains recent entries | ||
| /// Timer ids outlive queue removal, so the registry retains recent entries | ||
| /// after clear/fire just as Node retains the wrapper's prototype. The bounded | ||
| /// inventory avoids unbounded growth in long-running processes. | ||
| pub(crate) fn timer_constructor_value(id: i64) -> Option<f64> { | ||
| let kind = TIMER_HANDLE_KINDS.lock().unwrap().get(&id).copied()?; | ||
| let kind = timer_handle_kind(id)?; | ||
| let name = match kind { | ||
| CallbackTimerKind::Timeout => b"Timeout".as_slice(), | ||
| CallbackTimerKind::Immediate => b"Immediate".as_slice(), | ||
|
|
@@ -764,7 +739,7 @@ fn schedule_mock_callback_timer( | |
| let arg_handles = scope.root_nanbox_f64_slice(&args); | ||
| let delay = normalize_timer_delay(delay_ms); | ||
| let id = next_timer_id(); | ||
| record_timer_handle_kind(id, kind); | ||
| let scheduled = register_scheduled_timer(id, kind); | ||
| let due_ms = state.current_ms + delay as f64; | ||
| state.callbacks.push(MockCallbackTimer { | ||
| id, | ||
|
|
@@ -774,8 +749,8 @@ fn schedule_mock_callback_timer( | |
| args: crate::gc::RuntimeHandleScope::refreshed_nanbox_f64_slice(&arg_handles), | ||
| context: crate::async_context::capture_context(), | ||
| cleared: false, | ||
| _scheduled: scheduled, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '100,160p' crates/perry-runtime/src/timer/ref_states.rs
sed -n '720,850p' crates/perry-runtime/src/timer.rs
sed -n '580,620p' crates/perry-runtime/src/timer.rs
rg -n 'fn mock_timers_advance_to|call_timer_callback|MockCallbackTimer|struct ScheduledTimerId|fn retire|fn push_retired' crates/perry-runtime/src/timer.rs crates/perry-runtime/src/timer/ref_states.rsRepository: PerryTS/perry Length of output: 9985 🏁 Script executed: sed -n '1,135p' crates/perry-runtime/src/timer/ref_states.rs
sed -n '320,375p' crates/perry-runtime/src/timer.rs
sed -n '470,540p' crates/perry-runtime/src/timer.rs
rg -n -C 5 'timer_has_ref_state|timer_handle_kind|timer_constructor_value|is_known_timer_id|call_timer_callback|this\.hasRef|has_ref' crates/perry-runtime/src/timer.rs crates/perry-runtime/src/timer/ref_states.rsRepository: PerryTS/perry Length of output: 44411 🏁 Script executed: sed -n '1,135p' crates/perry-runtime/src/timer/ref_states.rs
sed -n '320,375p' crates/perry-runtime/src/timer.rs
sed -n '470,540p' crates/perry-runtime/src/timer.rs
rg -n -C 5 'timer_has_ref_state|timer_handle_kind|timer_constructor_value|is_known_timer_id|call_timer_callback|has_ref' crates/perry-runtime/src/timer.rs crates/perry-runtime/src/timer/ref_states.rsRepository: PerryTS/perry Length of output: 44411 Keep the mock callback registration alive during callback execution.
Keep Proposed fix- Some((
+ Some((
timer.id,
timer.callback,
timer.args.clone(),
timer.context.clone(),
+ None,
))
} else {
let timer = state.callbacks.remove(idx);
- Some((timer.id, timer.callback, timer.args, timer.context))
+ Some((
+ timer.id,
+ timer.callback,
+ timer.args,
+ timer.context,
+ Some(timer._scheduled),
+ ))
}
};
- if let Some((id, callback, args, context)) = action {
+ if let Some((id, callback, args, context, _scheduled)) = action {
call_timer_callback(id, callback, &args, &context);
}🤖 Prompt for AI Agents |
||
| }); | ||
| set_timer_ref_state(id, true); | ||
| Some(id) | ||
| } | ||
|
|
||
|
|
@@ -790,7 +765,7 @@ fn schedule_mock_interval_timer(callback: i64, interval_ms: f64, args: Vec<f64>) | |
| let arg_handles = scope.root_nanbox_f64_slice(&args); | ||
| let interval = normalize_timer_delay(interval_ms); | ||
| let id = next_timer_id(); | ||
| record_timer_handle_kind(id, CallbackTimerKind::Timeout); | ||
| let scheduled = register_scheduled_timer(id, CallbackTimerKind::Timeout); | ||
| let next_ms = state.current_ms + interval as f64; | ||
| state.intervals.push(MockIntervalTimer { | ||
| id, | ||
|
|
@@ -800,8 +775,8 @@ fn schedule_mock_interval_timer(callback: i64, interval_ms: f64, args: Vec<f64>) | |
| args: crate::gc::RuntimeHandleScope::refreshed_nanbox_f64_slice(&arg_handles), | ||
| context: crate::async_context::capture_context(), | ||
| cleared: false, | ||
| _scheduled: scheduled, | ||
| }); | ||
| set_timer_ref_state(id, true); | ||
| Some(id) | ||
| } | ||
|
|
||
|
|
@@ -840,10 +815,14 @@ fn mock_timers_advance_to(target_ms: f64) { | |
| }; | ||
| state.current_ms = due_ms; | ||
| if is_interval { | ||
| let timer = state.intervals[idx].clone(); | ||
| let interval = timer.interval_ms.max(1) as f64; | ||
| state.intervals[idx].next_ms = due_ms + interval; | ||
| Some((timer.id, timer.callback, timer.args, timer.context)) | ||
| let timer = &mut state.intervals[idx]; | ||
| timer.next_ms = due_ms + timer.interval_ms.max(1) as f64; | ||
| Some(( | ||
| timer.id, | ||
| timer.callback, | ||
| timer.args.clone(), | ||
| timer.context.clone(), | ||
| )) | ||
| } else { | ||
| let timer = state.callbacks.remove(idx); | ||
| Some((timer.id, timer.callback, timer.args, timer.context)) | ||
|
|
@@ -904,12 +883,7 @@ pub extern "C" fn js_timer_has_ref(timer_id: i64) -> i32 { | |
| // user explicitly called `.unref()` on the handle. Default `true` for | ||
| // any non-timer id is harmless since the dispatcher gates on | ||
| // `is_known_timer_id` first. | ||
| TIMER_REF_STATES | ||
| .lock() | ||
| .unwrap() | ||
| .as_ref() | ||
| .and_then(|s| s.states.get(&timer_id).copied()) | ||
| .unwrap_or(true) as i32 | ||
| timer_has_ref_state(timer_id) as i32 | ||
| } | ||
|
|
||
| #[no_mangle] | ||
|
|
@@ -1087,7 +1061,7 @@ fn schedule_callback_timer( | |
| let deadline = Instant::now() + Duration::from_millis(delay_ms); | ||
|
|
||
| let id = next_timer_id(); | ||
| record_timer_handle_kind(id, kind); | ||
| let scheduled = register_scheduled_timer(id, kind); | ||
|
|
||
| let mut context = crate::async_context::capture_context(); | ||
| let context_roots = crate::async_context::root_snapshot(&scope, &context); | ||
|
|
@@ -1118,8 +1092,8 @@ fn schedule_callback_timer( | |
| cleared: false, | ||
| // #6185: the scheduling agent owns the callback closure + args. | ||
| owner: crate::agent::current_agent(), | ||
| _scheduled: scheduled, | ||
| }); | ||
| set_timer_ref_state(id, true); | ||
|
|
||
| id | ||
| } | ||
|
|
@@ -1263,7 +1237,7 @@ pub extern "C" fn js_callback_timer_tick() -> i32 { | |
| |timer| { | ||
| crate::agent::owns(timer.owner) | ||
| && timer.deadline <= now | ||
| && (timer_has_ref_state(timer.id) || allow_unref) | ||
| && (allow_unref || timer_has_ref_state(timer.id)) | ||
| }, | ||
| ) | ||
| }; | ||
|
|
@@ -1444,7 +1418,7 @@ pub extern "C" fn js_callback_timer_next_deadline() -> f64 { | |
| .unwrap() | ||
| .iter() | ||
| .filter(|t| { | ||
| !t.cleared && crate::agent::owns(t.owner) && (timer_has_ref_state(t.id) || allow_unref) | ||
| !t.cleared && crate::agent::owns(t.owner) && (allow_unref || timer_has_ref_state(t.id)) | ||
| }) | ||
| .map(|t| { | ||
| if t.deadline <= now { | ||
|
|
@@ -1573,6 +1547,8 @@ struct IntervalTimer { | |
| cleared: bool, | ||
| /// #6185: agent that owns `callback` / `args`. See `CallbackTimer::owner`. | ||
| owner: crate::agent::AgentId, | ||
| /// #10447: see `CallbackTimer::_scheduled`. | ||
| _scheduled: ScheduledTimerId, | ||
| } | ||
|
|
||
| // SAFETY: see `CallbackTimer` — the owner tag plus owner-filtered ticking is | ||
|
|
@@ -1604,7 +1580,7 @@ fn schedule_interval_timer(callback: i64, interval_ms: f64, args: Vec<f64>) -> i | |
| let next_deadline = Instant::now() + Duration::from_millis(interval); | ||
|
|
||
| let id = next_timer_id(); | ||
| record_timer_handle_kind(id, CallbackTimerKind::Timeout); | ||
| let scheduled = register_scheduled_timer(id, CallbackTimerKind::Timeout); | ||
|
|
||
| let mut context = crate::async_context::capture_context(); | ||
| let context_roots = crate::async_context::root_snapshot(&scope, &context); | ||
|
|
@@ -1623,8 +1599,8 @@ fn schedule_interval_timer(callback: i64, interval_ms: f64, args: Vec<f64>) -> i | |
| cleared: false, | ||
| // #6185: the scheduling agent owns the callback closure + args. | ||
| owner: crate::agent::current_agent(), | ||
| _scheduled: scheduled, | ||
| }); | ||
| set_timer_ref_state(id, true); | ||
|
|
||
| id | ||
| } | ||
|
|
@@ -1700,7 +1676,7 @@ pub extern "C" fn js_interval_timer_tick() -> i32 { | |
| if !timer.cleared | ||
| && crate::agent::owns(timer.owner) | ||
| && timer.next_deadline <= now | ||
| && (timer_has_ref_state(timer.id) || allow_unref) | ||
| && (allow_unref || timer_has_ref_state(timer.id)) | ||
| { | ||
| callbacks.push(( | ||
| timer.id, | ||
|
|
@@ -1788,7 +1764,7 @@ pub extern "C" fn js_interval_timer_next_deadline() -> f64 { | |
| .unwrap() | ||
| .iter() | ||
| .filter(|t| { | ||
| !t.cleared && crate::agent::owns(t.owner) && (timer_has_ref_state(t.id) || allow_unref) | ||
| !t.cleared && crate::agent::owns(t.owner) && (allow_unref || timer_has_ref_state(t.id)) | ||
| }) | ||
| .map(|t| { | ||
| if t.next_deadline <= now { | ||
|
|
@@ -1933,7 +1909,7 @@ mod tests_inline; | |
| #[cfg(test)] | ||
| pub(crate) use tests_inline::*; | ||
|
|
||
| /// `PERRY_GC_CENSUS`: the three timer queues. | ||
| /// `PERRY_GC_CENSUS`: the three timer queues and the id registry. | ||
| pub(crate) fn timer_tables_census() -> Vec<crate::gc::census::SideTableRow> { | ||
| use crate::gc::census::vec_bytes; | ||
| let mut rows = Vec::new(); | ||
|
|
@@ -1948,5 +1924,6 @@ pub(crate) fn timer_tables_census() -> Vec<crate::gc::census::SideTableRow> { | |
| let inner: usize = v.iter().map(|t| vec_bytes(&t.args)).sum(); | ||
| rows.push(("timer.interval_timers", v.len(), vec_bytes(&v) + inner)); | ||
| } | ||
| rows.push(ref_states::ref_states_census()); | ||
| rows | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe
unref()accurately.Line 3 says that the program detached the callback.
unref()does not detach or cancel a callback. It only permits process exit when no referenced work remains. State that eviction kept the process alive and therefore allowed the unref'd callback to run.Proposed correction
📝 Committable suggestion
🤖 Prompt for AI Agents