-
Notifications
You must be signed in to change notification settings - Fork 1
Timers fixes #6
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
Timers fixes #6
Conversation
The test naturally fails, as the current implementation actually only handles string callbacks that are eval-ed, but preserving some reference to a closure (with proper context binding) does not seems to be trivial (due to lack of the expertise on the project on my side).
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.
Pull Request Overview
This PR replaces the global timer queue with a per-Runtime timer manager and introduces a new TimerQueue implementation.
- Refactor
Runtimeto hold anOption<TimersRuntime>instead of a global flag and functions - Update
Runtimemethods (build_from_config,resolve_pending_jobs,has_pending_jobs) to use the newTimersRuntimeAPI - Add a heap-backed
TimerQueuewith basic operations and tests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/javy/src/runtime.rs | Refactored timer support from globals to Option<TimersRuntime> and removed the old flag and functions |
| crates/javy/src/apis/timers/queue.rs | Added TimerQueue implementation with heap logic and initial tests |
Comments suppressed due to low confidence (3)
crates/javy/src/runtime.rs:214
- Removing the public
has_timers_enabledmethod is a breaking API change; consider reintroducing it as a deprecated alias or updating all callers.
pub fn has_timers_enabled(&self) -> bool {
crates/javy/src/apis/timers/queue.rs:89
- [nitpick] There are no tests covering
get_expired_timers. Add unit tests to verify that timers fire at the correct time and repeating timers are re-enqueued as expected.
pub fn get_expired_timers(&mut self) -> Vec<Timer> {
crates/javy/src/apis/timers/queue.rs:9
- [nitpick] The
Functionvariant inTimerCallbackis ambiguous; consider renaming it to something more descriptive (e.g.,FunctionReforCallbackFn).
Function,
|
|
||
| pub fn remove_timer(&mut self, timer_id: u32) -> bool { | ||
| let original_len = self.timers.len(); | ||
| self.timers.retain(|timer| timer.id != timer_id); |
Copilot
AI
May 28, 2025
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.
Calling .retain on a BinaryHeap breaks its internal heap invariant. Instead, filter elements and rebuild the heap (e.g., BinaryHeap::from(filtered_vec)), or use a data structure that supports efficient removal.
| self.timers.retain(|timer| timer.id != timer_id); | |
| let filtered_timers: Vec<_> = self.timers.drain().filter(|timer| timer.id != timer_id).collect(); | |
| self.timers = BinaryHeap::from(filtered_timers); |
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.
- I see no such comments in the docs https://doc.rust-lang.org/std/collections/struct.BinaryHeap.html#method.retain
- They even fixed potential invariant breakage in case of a panic Rebuild BinaryHeap on unwind from retain rust-lang/rust#106918
From that I conclude that the comment is misleading.
…from 2025-01-03 to 2025-05-28 - Reference PR #6 from id-ilych/fix-timers for timer improvements - Provide proper attribution for concurrent changes
DISCLAIMER:
I have no expertise in the project and no expertise in rust, so it is rather experiment/exploration/playground.
I did not bother checking timers' specification, but it should be fine given the current goals.
Also, the implementation currently stores callback function in JS'
globalswhich is not ideal, but I haven't found a way to do it on rust level (yet).It seems, timer tests are not flaky now
Description of the change
Why am I making this change?
Checklist
javy-cliandjavy-plugindo not require updating CHANGELOG files.