Add support for timers/promises module from nodejs

[WhiteAutumn]

Purpose (TL;DR)

Fix issue #469 by implementing promise-based wrappers around setTimeout, setImmediate, and setInterval.

Solution

This PR takes a similar approach to #467 by @swenzel-arc of detecting the availability of the timers/promises module by attempting to import it and then overwriting the functions on the import object, which should be the same object users have, given they do not mess with the module cache. Like the aforementioned PR, the functions are only overwritten when calling install() on the globalObject.

Limitations

• When aborting a timer using an AbortSignal, the error thrown is not the same as the one thrown by node. This is due to the AbortError error being publicly available. See this issue.
• This PR does not touch scheduler.wait or scheduler.yield of the timers/promises module as they are marked as experimental.

[fatso83]

Very solid work! Love the implementation and the tests. Just had a minor comment. You can see what you think of it before I approve. It's just a minor quibble.

[benjamingr]

There are some leaks in the signal (e.g. we don't remove the event listener when fake timers are uninstalled) but generally that's fine for the first version so lgtm.

[fatso83]

Good note about the removal of event listeners. When I do any development on Sinon projects, I tend to make use of the watch feature of the test runner. That's where stuff like not cleaning up properly tends to show itself.

If it's not too much problem, I'd like to see that being done.

[WhiteAutumn]

Sure, I can take a look at it. Just so we're on the same page, we're talking about the scenario where while a timer is running, an abort signal has been passed, and the clock is uninstalled before the timer fires, leading to it never firing and the therefore the abort signal listener never being removed?

This is something I have missed, and the listeners should absolutely be removed in that scenario, but for what it's worth I don't think it would lead to memory leaks while running tests in watch mode. As long as no references remain to the abort signal, it should be garbage collected and the event listener along with it.

As to how to solve this, I would like a second opinion on how to approach it. The way I see it this would require something akin to an onUninstall event that the promise based implementations can listen to and then remove the abort event listeners when fired. It doesn't feel great adding more event listeners to solve hanging event listeners but I can't think of any other way of doing it. What do you think?

[fatso83]

You know what, I don't have available mental capacity (or time) at the moment to dig deeply into this, and if it really isn't an issue in practice, then I have no issues merging this now. There's always room for another PR later, if you find a nice way that isn't too complicated.

[u873838]

Any idea when a new release will be cut with this change?

[fatso83]

Thanks for the reminder :)

[benjamingr]

This is something I have missed, and the listeners should absolutely be removed in that scenario, but for what it's worth I don't think it would lead to memory leaks while running tests in watch mode. As long as no references remain to the abort signal, it should be garbage collected and the event listener along with it.

References can remain to the signal (e.g. the signal is the result of AbortSignal.any of a local signal and a process-level signal.

It's fine for it to happen at a follow up PR. Generally you did cleanup a lot better than I expected in this PR in the first place and it's a relatively small miss. I review a lot of AbortSignal library code so this is a compliment :)

As to how to solve this, I would like a second opinion on how to approach it. The way I see it this would require something akin to an onUninstall event that the promise based implementations can listen to and then remove the abort event listeners when fired. It doesn't feel great adding more event listeners to solve hanging event listeners but I can't think of any other way of doing it. What do you think?

We keep a list/map or all signal and event listeners associated with the clock instance and when the clock is uninstalled we call that?

// Somewhere when an event listener is added
signal.addEventListener('abort', listener, options); 
clock.listners.push({ signal, listener });

// In uninstall
clock.listeners.forEach(({ signal, listener }) => signal.removeEventListener(listener))

[WhiteAutumn]

References can remain to the signal (e.g. the signal is the result of AbortSignal.any of a local signal and a process-level signal.

Ohh, I see. Yeah then it's definitely something that would be good to get fixed before a release.

We keep a list/map or all signal and event listeners associated with the clock instance and when the clock is uninstalled we call that?

Thanks for the input. I'll proceed with that kind of a solution and make a separate PR.

[fatso83]

@u873838 Thanks to White Autumns swift second PR this is already out.