Skip to content
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

Using Timeout/Interval with millis >= i32::MAX results in incorrect behavior #121

Open
arthurprs opened this issue Mar 15, 2021 · 1 comment
Labels
bug Something isn't working

Comments

@arthurprs
Copy link

arthurprs commented Mar 15, 2021

Describe the Bug

Millisecond arguments are taken as u32 and internally cast (unchecked) to i32. Leading to the timer/interval firing after the wrong duration.

Look for millis as i32, in the examples bellow.

Example 1.

pub fn new<F>(millis: u32, callback: F) -> Timeout
where
F: 'static + FnOnce(),
{
let closure = Closure::once(callback);
let id = GLOBAL.with(|global| {
global
.set_timeout_with_callback_and_timeout_and_arguments_0(
closure.as_ref().unchecked_ref::<js_sys::Function>(),
millis as i32,
)

Example 2.

pub fn new<F>(millis: u32, callback: F) -> Interval
where
F: 'static + FnMut(),
{
let closure = Closure::wrap(Box::new(callback) as Box<dyn FnMut()>);
let id = GLOBAL.with(|global| {
global
.set_interval_with_callback_and_timeout_and_arguments_0(
closure.as_ref().unchecked_ref::<js_sys::Function>(),
millis as i32,
)

Steps to Reproduce

Use the timer/interval with (u32::MAX / 2) as the duration, they'll fire immediately.

Expected Behavior

Either

  1. Panic if duration is larger than the JS runtime supports.
  2. Error result (breaking change)
  3. Clamping it to 0..=i32::MAX before sending to the JS runtime.

Actual Behavior

See description.

Additional Context

N/A

@arthurprs arthurprs added the bug Something isn't working label Mar 15, 2021
@DesmondWillowbrook
Copy link

DesmondWillowbrook commented Jun 11, 2021

Looks like this is a very old bug in the JS spec?

https://stackoverflow.com/questions/3468607/why-does-settimeout-break-for-large-millisecond-delay-values

Should we patch over this behavior? Since it will be deviating from default JS behavior which is to fire immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants