Skip to content

Conversation

@id-ilych
Copy link
Contributor

@id-ilych id-ilych commented May 23, 2025

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' globals which is not ideal, but I haven't found a way to do it on rust level (yet).

  • Don't use global variable for timers queue
    It seems, timer tests are not flaky now
  • Refine the code
  • Add support for function callbacks (and related test to cover)

Description of the change

Why am I making this change?

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-plugin do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

id-ilych added 7 commits May 23, 2025 23:33
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).
@id-ilych id-ilych marked this pull request as ready for review May 28, 2025 07:47
Copilot AI review requested due to automatic review settings May 28, 2025 07:47
Copy link

Copilot AI left a 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 Runtime to hold an Option<TimersRuntime> instead of a global flag and functions
  • Update Runtime methods (build_from_config, resolve_pending_jobs, has_pending_jobs) to use the new TimersRuntime API
  • Add a heap-backed TimerQueue with 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_enabled method 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 Function variant in TimerCallback is ambiguous; consider renaming it to something more descriptive (e.g., FunctionRef or CallbackFn).
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);
Copy link

Copilot AI May 28, 2025

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.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I see no such comments in the docs https://doc.rust-lang.org/std/collections/struct.BinaryHeap.html#method.retain
  2. 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.

@id-ilych id-ilych merged commit 7fa0348 into workato:main May 28, 2025
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
timanovsky added a commit that referenced this pull request May 28, 2025
@id-ilych id-ilych changed the title [WIP] Timer fixes Timers fixes May 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant