Timers fixes #6
Merged
Timers fixes #6
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
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.
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); |
There was a problem hiding this comment.
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.
Suggested change
| 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); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
- 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.
timanovsky
added a commit
that referenced
this pull request
May 28, 2025
…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
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.
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.