Cancelable Timeouts/Intervals

[simbleau]

Summary

Synchronously, we get an ID from a Timeout::new().forget() or Interval::new().forget().
Is there a reason we have to consume the object and return a u64 ID? I don't think so.

I believe a better solution would be to have an inner field on the object, id: Option<u64> which holds such an ID if it was forgot()'ed or submitted async.

And hence, then we could implement cancel() after it has been sent as a hook to web_sys::cancel_timeout_with_handle(&self: &Window, id: u64)

Motivation

The motivation is that (in my opinion) it's not straightforward to cancel a Timeout or Interval for the average user, and this is not ergonomic as a result.

Detailed Explanation

TLDR:

pub struct Timeout<F: FnOnce> { 
    id: Option<u64>,
    f: F,
}

impl<F: FnOnce> Timeout<F> {
    pub fn new(f: F) -> Self { ... }
    pub fn forget(&mut self) {
        let id = web_sys::submit_timeout.... // Whatever it is currently
        self.id = Some(id)
    }

    pub fn cancel(self) { 
        if let Some(id) = self.id {
            self.id = None;
            web_sys::window().map(|w| w.cancel_timeout_with_handle(id)).expect_throw("No window")
        }
    }

    pub fn id(&self) -> Option<u64> { 
        self.id
    }
}

Drawbacks, Rationale, and Alternatives

It would be a breaking change.

[ranile]

Timer/Interval are cancelled when they're dropped. Is that not enough?

Side note: you can use std::men:::forget instead of forget and achieve the same results

[simbleau]

The use case which made me think this wasn't enough was a project I'm making.

I use an effect (Yew) which changes images on an image carousel every X seconds. The interval acts as the timer. Whenever the image carousel isn't being rendered anymore (the tab is not focused or the page in a SPA), the interval continues to waste resources. So in my effect's destructor, I need to manually cancel the interval/timeout after it has started, which is not as ergonomic as it should be.

Overall I think this is a relatively good example of why it should be easier to work with intervals or timers, since it doesn't need to require as much triable knowledge for the user.

[ranile]

I think adding a method that cancels the Timeout/Interval without consuming the object is a really bad API design. Can you show the code where you ran into this issue?

…

On Mon, Sep 19, 2022, 12:06 AM simbleau ***@***.***> wrote: The use case which made me think this wasn't enough was a project I'm making. Onload, I use an effect (Yew) which changes images on a carousel every X seconds. Whenever the image carousel isn't being rendered anymore (the tab is not focused or the page in a SPA), the interval continues. So instead, in my effect's destructor, I need to manually cancel the interval/timeout after it has started. — Reply to this email directly, view it on GitHub <#254 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALJJ7WOQIWLK66RMLBERJYLV65R5DANCNFSM6AAAAAAQPOFOJM> . You are receiving this because you commented.Message ID: ***@***.***>

[simbleau]

I have modified the original example to consume on cancel. Not sure why I made it use &mut self.

Here is a code example:

// The image src's in use for our carousel
#[derive(Properties, PartialEq)]
pub struct Props {
    pub images: Vec<&'static str>
}

#[function_component(ImageCarousel)]
fn view(props: &Props) -> Html {
    // Create references to each image
    let img_refs = Rc::new(
        props
            .images
            .iter()
            .map(|_| use_node_ref())
            .collect::<Vec<NodeRef>>(),
    );

    // If there are more than 2 images, the image carousel cycles through
    // images while being rendered
    if img_refs.len() >= 2 {
        let img_refs = img_refs.clone();
        use_effect(move || {
            // We need to store all the intervals to cancel at cleanup for the effect
            let intervals = Rc::new(RefCell::new(vec![]));

            for (i, img_node) in img_refs
                .iter()
                .map(|img_ref| img_ref.cast::<HtmlElement>())
                .enumerate()
            {
                if let Some(img) = img_node {
                    // Carousel logic here - Show/hide on interval
                    intervals.borrow_mut().push(
                        Interval::new(3000, {
                            // Show image_refs[i]
                            // Wait
                            // Hide image_refs[i]
                        })
                    );
                }
            }

            // Destructor - Clear all intervals
            let intervals = intervals.clone();
            move || {
                web_sys::window()
                    .map(|window| {
                        for i in intervals.borrow().iter() {
                            window.clear_interval_with_handle(*i);
                        }
                    })
                    .unwrap();
            }
        });
    }

    // Render
    let images = props
        .images
        .iter()
        .enumerate()
        .map(|(i, img_src)| {
            html! {
                <img src={ *img_src } ref={ &img_refs[i] } />
            }
        })
        .collect::<Html>();
    html({ images })
}

[simbleau]

If we have the ability to cancel intervals, we'd have some nicer ergonomics:

        // use_effect()'s destructor
        move || {
            for interval in intervals {
                interval.cancel()
            }
        }

[futursolo]

If we have the ability to cancel intervals, we'd have some nicer ergonomics:

        // use_effect()'s destructor
        move || {
            for interval in intervals {
                interval.cancel()
            }
        }

Since you have an RefCell<Vec<Interval>>, you can use intervals.take() to cancel all intervals.

In general, I think a non-destructive version can already be covered by Option<Interval>::take().
For this specific example, the same behaviour can be achieved by dropping the intervals in the destructor:

// The image src's in use for our carousel
#[derive(Properties, PartialEq)]
pub struct Props {
    pub images: Vec<&'static str>
}

#[function_component(ImageCarousel)]
fn view(props: &Props) -> Html {
    // Create references to each image
    // use_memo is not available in 0.19, but can be replaced with creating a new Rc<Vec<NodeRef>> everytime.
    let img_refs = use_memo(|images| images.iter().map(|_| NodeRef::new()), images.clone());

    use_effect_with_deps(|img_refs| {
        // If there are more than 2 images, the image carousel cycles through
        // images while being rendered
        let intervals = (img_refs.len() >= 2).then(move || {
            img_refs
                .iter()
                .map(|img_ref| img_ref.cast::<HtmlElement>())
                .filter_map(|m| m)
                .enumerate()
                .map(|(i, img)| Interval::new(3000, move || { 
                    // Show image_refs[i]
                    // Wait
                    // Hide image_refs[i]
                }))
                .collect::<Vec<_>>()
            });

            // Destructor - Clear all intervals
            move || {
                let _intervals = intervals;
            }
        });
    }, img_refs.clone());

    // Render
    let images = props
        .images
        .iter()
        .zip(img_refs.iter())
        .map(|(img_src, img_ref)| {
            html! {
                <img src={*img_src} ref={img_ref} key={*img_src} />
            }
        })
        .collect::<Html>();
    
    images
}

[simbleau]

This is some awesome code, but should there not be a way to consume and cancel it explicitly?

Sure you could call mem::drop, but to future users I don't believe that is always obvious - You have to know the behavior there exists.

[futursolo]

This is some awesome code, but should there not be a way to consume and cancel it explicitly?

Sure you could call mem::drop, but to future users I don't believe that is always obvious - You have to know the behavior there exists.

I think this behaviour is used widely in both std and third-party crates and users should become adapted to it as they get used to the Rust ecosystem.
e.g.: std::cell::Ref, std::sync::MutexGuard, std::fs::File, tokio::runtime::EnterGuard...

[ranile]

Those structs are based on the RAII pattern. Drop means that timeout/interval is cancelled. I think we can do a better job of documenting the Drop behavior. Would anyone like to PR?

…

On Wed, Sep 21, 2022, 5:53 PM Kaede Hoshikawa ***@***.***> wrote: This is some awesome code, but should there not be a way to consume and cancel it explicitly? Sure you could call mem::drop, but to future users I don't believe that is always obvious - You have to know the behavior there exists. I think this behaviour is used widely in both std and third-party crates and users should become adapted to it as they get used to the Rust ecosystem. e.g.: std::cell::Ref, std::sync::MutexGuard, std::fs::File, tokio::runtime::EnterGuard... — Reply to this email directly, view it on GitHub <#254 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALJJ7WM3LJTFVPY7WZ5CHU3V7MANPANCNFSM6AAAAAAQPOFOJM> . You are receiving this because you commented.Message ID: ***@***.***>

[simbleau]

I think the code is fine. Good points. However I think the drop behavior and RAII pattern used here should be documented, as hamza suggests.

I would like to improve the docs. I found it strange forget() makes it "uncancelable," since it can still be cancelled (via mem::drop/clearTimeout/clearInterval).

gloo/crates/timers/src/callback.rs

Lines 126 to 127 in 1cd7b5c

	/// Once scheduled, you can either `cancel` so that it ceases to fire or `forget`
	/// it so that it is un-cancel-able.

gloo/crates/timers/src/callback.rs

Lines 173 to 177 in 1cd7b5c

	/// Make this interval uncancel-able.
	///
	/// Returns the identifier returned by the original `setInterval` call, and
	/// therefore you can still cancel the interval by calling `clearInterval`
	/// directly (perhaps via `web_sys::clear_interval_with_handle`).

I will volunteer to submit a PR shortly.

[simbleau]

This issue can be closed upon merge of #255.