SetInterval does not clear on HMR reload / code change

[nikhil-swamix]

Describe the bug

this i use to observe changed value
$: {
filterSearchResults(searchTerm);
console.log(selections);
}
------ but this goes out of control and keeps printing forever
setInterval(() => {
console.log(selections);
}, 1000);

Reproduction

this i use to observe changed value
$: {
filterSearchResults(searchTerm);
console.log(selections);
}
------ but this goes out of control and keeps printing forever
setInterval(() => {
console.log(selections);
}, 1000);

Logs

No response

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (12) x64 Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz
    Memory: 7.16 GB / 15.94 GB
  Binaries:
    Node: 20.6.1 - C:\Program Files\nodejs\node.EXE       
    Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD        
    npm: 9.8.1 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.7.6 - ~\AppData\Roaming\npm\pnpm.CMD
  Browsers:
    Edge: Chromium (120.0.2210.91)
  npmPackages:
    @sveltejs/adapter-auto: ^3.0.1 => 3.0.1 
    @sveltejs/adapter-node: ^2.0.1 => 2.0.1
    @sveltejs/kit: ^2.0.4 => 2.0.4
    @sveltejs/vite-plugin-svelte: ^3.0.0 => 3.0.1
    svelte: ^4.2.8 => 4.2.8
    vite: ^5.0.10 => 5.0.10

Severity

serious, but I can work around it

Additional Information

[Conduitry]

Transferring this to the HMR repository. However, as mentioned on the similar issue sveltejs/sites#146 I'm not really sure what would be a reasonable way to resolve this.

[rixo]

HMR just destroys and recreates your component. If you have a leaking setInterval that keeps running, you would have the same problem in production. HMR just makes it more obvious, because components might get recreated less often, or never destroyed at all in production.

As a general rule that suffers few exceptions, you should make sure that your components always clean after themselves when they get destroyed, and don't leave things like setInterval running.

The general pattern is pretty basic, just implement such resources holders in onMount and use the cleanup callback (return value) to release them:

onMount(() => {
  const interval = setInterval(() => { ... })

  return () => {
    clearInterval(interval)
  }
}

Note that having setInterval calls directly at the root of your components instead of an onMount callback, like your example seems to suggest, would not play nice with SSR either. It would probably end up eating all your server's memory over time.

[nikhil-swamix]

HMR just destroys and recreates your component. If you have a leaking setInterval that keeps running, you would have the same problem in production. HMR just makes it more obvious, because components might get recreated less often, or never destroyed at all in production.

As a general rule that suffers few exceptions, you should make sure that your components always clean after themselves when they get destroyed, and don't leave things like setInterval running.

The general pattern is pretty basic, just implement such resources holders in onMount and use the cleanup callback (return value) to release them:

onMount(() => {
  const interval = setInterval(() => { ... })

  return () => {
    clearInterval(interval)
  }
}

Note that having setInterval calls directly at the root of your components instead of an onMount callback, like your example seems to suggest, would not play nice with SSR either. It would probably end up eating all your server's memory over time.

---

i see, i was able to arrive at the same conclusion, and later used the clear interval in onDestroy....
i have a proposal let me know if its feasible.

1. creating a scoped set interval like the other variables in component, by modding the original setInterval,
2. for example window.setInterval is different than set interval
3. this way by some "internal modding" it will be auto managed for set and clear intervals, and maybe some other commonly used functions which subject to leaks.
4. this will result in improved dev exp, and avoid pitfalls, and fallback to window global methods if anyone really wants it.

@rixo @Rich-Harris , this is just primitive idea, will it consider a bad architecture/practice? let me know.