chrisguttandin / worker-timers

A replacement for setInterval() and setTimeout() which works in unfocused windows.
MIT License
582 stars 25 forks source link

There is no timeout scheduled with the given id "386". #433

Closed LavanyaBurlagadda closed 4 months ago

LavanyaBurlagadda commented 1 year ago

We are getting this issue when we are clearing the timeouts in the component unmounting stage.

Let's say that even if the timeout is executed if we try to clear the timeout it shouldn't throw an error.

We are getting sentry issues reported for this issue.

image

chrisguttandin commented 1 year ago

There is a subtle difference in how worker-timers handles missing missing ids.

https://github.com/chrisguttandin/worker-timers#error-handling

Is it possible that you call clearTimeout() twice for the same id?

In any case it would be helpful to see the code which registers and clears the timeout.

theogravity commented 1 year ago

I seem to be having this problem too.

Here's some sample react code:

import { clearInterval, setInterval } from 'worker-timers';
import React, { useCallback, useEffect, useRef } from 'react';

export const Component = () => {
  const refreshExpiryTimer = useRef<number | null>(null);

  const clearRefreshExpiryTimer = useCallback(() => {
    if (refreshExpiryTimer.current) {
      try {
        // NOTE: this is not window.clearTimeout, this is from the worker-timers package
        // worker-timers version can throw if the id is invalid
        clearInterval(refreshExpiryTimer.current);
      } catch (e) {
        // ignore
      }
    }
  }, [refreshExpiryTimer]);

 useEffect(() => {
    clearRefreshExpiryTimer();
    refreshExpiryTimer.current = setInterval(() => {
        //...
    }, 10000);

    return () => {
      clearRefreshExpiryTimer();
    };
  }, [clearRefreshExpiryTimer]);

  return null
}

It'd be really nice to just have an option to ignore any errors around this or add a function that allows us to check if the id exists or not

chrisguttandin commented 1 year ago

I'm not 100% sure how this could be done best in React but I think refreshExpiryTimer needs to be reset to null after calling clearInterval(refreshExpiryTimer.current);. Otherwise it will probably try to call clearInterval() with the same number again.

By the way, it's theoretically possible that the number returned by setInterval() is 0. Therefore (refreshExpiryTimer.current) might resolve to false even though its a number.

theogravity commented 1 year ago

I just had that thought now and was going to mention that then saw your comment.

Nulling it out should resolve it. Thanks!

theogravity commented 1 year ago

When I tried my code in a skeleton sandbox, it's fine, but when I use it in my own app, I get the issue.

I noticed that when I log out the variable that stores the timer, it starts with a low count then increments for each setup and teardown in the component. This looks like proper behavior.

https://codesandbox.io/s/worker-timer-bug-r7qyyx

In the bug I'm seeing, the error shows ids often in the three or four digits. eg There is no timeout scheduled with the given id "2164".

I do use worker-timers in other scripts of our app, but I don't think we're creating timers that would get to the thousands assuming id increments for each timer created.

I attempted to see if using it in two different components had some impact in the sandbox above, but again, I don't see anything wrong there.

Edit: Apparently we do use this package in lots of places, but I don't think into the four digits worth of setInterval / setTimeout cycles

theogravity commented 1 year ago

How is the timerId generated? Looking at https://github.com/chrisguttandin/worker-timers-worker, it doesn't seem obvious to me.

I also noticed the last update on https://github.com/chrisguttandin/worker-timers/blob/master/src/worker/worker.ts was 2 years ago, not sure if this is actively used or not, but was wondering if it could be due to stale code.

Edit:

I see the timerId gets assigned here

https://github.com/chrisguttandin/worker-timers-broker/blob/master/src/module.ts#L113C14-L113C14

theogravity commented 1 year ago

lol, I figured it out, I forgot to null out in another file where I've also started to use the package

chrisguttandin commented 1 year ago

@theogravity Great, I'm glad you found the issue. As a back story, the ids are generated using fast-unique-numbers. I created that utility because I needed this functionality in various places. As long as you don't use more than 1073741824 timers it will only increase the number one. After that it will try to employ more intelligent techniques. :-)

@LavanyaBurlagadda Is your problem of similar nature?

ahoyahoy commented 4 months ago

Hello, I am facing the same problem. The problem is with strict react mode, where render is called twice. (Because I don't have this problem when I switch responseStrictMode to false in next.config.js) I also tried to catch these errors with try { clearTimeout(..) } catch() {} but it doesn't work.

chrisguttandin commented 4 months ago

I finally removed this behavior in v8. worker-timers should no longer throw an error when calling clearInterval() or clearTimeout() with an unknown id.

Please let me know if the problem still persists.