`clearTimeout` will not clear timers created prior to clock installation

[JadenSimon]

We understand you have a problem and are in a hurry, but please provide us with some info to make it much more likely for your issue to be understood, worked on and resolved quickly.

• FakeTimers version : 8.0.1
• Environment : node12, macOS Catalina 10.15.7
• Other libraries you are using: several, this is happening in tests for a VS Code extension

What did you expect to happen? clearTimeout to clear timers even if they weren't created with the mock.

What actually happens The timer that should have been cleared still fires.

How to reproduce

Describe with code how to reproduce the faulty behaviour or link to code on JSBin or similar

const timer = setTimeout(() => console.log('This should not be called'), 1);
const clock = FakeTimers.install();
clearTimeout(timer);
clock.uninstall();
// ... wait 1 ms + next event loop
// > 'This should not be called'

I can make a PR to fix this if the current behavior is not intended. I added a patch to my own source, though a patch here would be much more robust (need to consider things like duplicate handles, clearInterval, clearImmediate, etc.)

[calebboyd]

Came here to file the same bug. Ran into it with pg.Pools idleTimeout firing despite being cleared.

[fatso83]

I cannot see how things created outside of a dependency is to be handled by the dependency is expected behavior at all, but I see how it is useful to "auto-cleanup" after setting up the fake timer version. It's not a bug from my perspective, but it is a useful new feature with a breaking change. I guess simply delegating the call to the original implementation should be fine in most cases, though, so it could be done as the default behavior, but we would need a flag to disable it. Help is welcome.

[benjamingr]

I agree it's not a bug and I am not sure Sinon should address it. If you call an API before you mock it it is unreasonable for the mocks to be expected to intercept it.

I guess that a fix here on the Sinon side (or at least a warning) is probably fine?

[calebboyd]

I agree with these sentiments. Fortunately it was straight forward to workaround once it was discovered. A warning about existing timers would definitely have sped up that process though

[JadenSimon]

I made this issue more as a feature request rather than necessarily a bug report. I think respecting the expectations of older timers is useful functionality, though it might not quite fit within this library. I was leaning more towards an opt-in flag to enable this behavior rather than it be the default. Also, a warning at the very least serves to make debugging less obnoxious, otherwise things can randomly break with no clear reason why (this was the most frustrating part).

Just a little more about the context in which I'm using Sinon in:

I work on an extension for VS Code where many of our 'unit tests' are more like mini-integration tests. It is cumbersome to stub/mock every dependency since we essentially live inside our dependency. So these APIs still run in the background, occasionally breaking during tests because of clearTimeout being replaced. There's ways around this, but for us the cleanest way is to preserve clearTimeout behavior for pre-existing timers. In truth our tests should've been written with more separation in mind, but that was before my time.

[benjamingr]

Opt in flag is probably fine, so is a warning. I am in favor of either one.

[fatso83]

Totatlly pro opt-in. Not sure how we are to detect the presence of existing timers, though? I do not know of any such standardized feature.

node, Symbol(triggerId):

> setTimeout(()=>console.log("hei"), 15)
Timeout {
  _idleTimeout: 15,
  _idlePrev: [TimersList],
  _idleNext: [TimersList],
  _idleStart: 33222,
  _onTimeout: [Function (anonymous)],
  _timerArgs: undefined,
  _repeat: null,
  _destroyed: false,
  [Symbol(refed)]: true,
  [Symbol(kHasPrimitive)]: false,
  [Symbol(asyncId)]: 140,
  [Symbol(triggerId)]: 5
}

browsers; incremental id:

setTimeout(()=>console.log("hei"), 15)
1
setTimeout(()=>console.log("hei"), 15)
2

[benjamingr]

@fatso83 I think the idea is that clearTimeout/clearInterval will just also delegate to the native versions of them.

[fatso83]

@benjamingr I got the implementation bits, I just did not get how/what the timers would warn against. Ref

A warning about existing timers would definitely have sped up that process though

If that meant "if any existing timers has been executed using the built-ins, make Sinon/fake-timers print a warning about this when creating the first new fake one" I was not sure how we were to detect this. If this is not what we are to print a warning about, then I am not sure what we are to warn about 😄

[benjamingr]

Oh, I figured something much more naive where if you clearTimeout a timer that doesn't exist then it will warn (and to start counting timer ids from a very large number like 1e15 so that there are no likely collisions)

[JadenSimon]

Oh, I figured something much more naive where if you clearTimeout a timer that doesn't exist then it will warn (and to start counting timer ids from a very large number like 1e15 so that there are no likely collisions)

This is what I was thinking as well. Any handle below the initial fake counter can be assumed to be a real handle. So we should warn by default, or delegate to the native clearTimeout/clearInterval if the user has opted-in to the automatic clean-up behavior. Would an option name like shouldHandleNativeTimers make sense? Seems verbose but trying to keep the same naming pattern as the other flag shouldAdvanceTime.

[fatso83]

Closed by #407