Timer not cleaned up when cancelled.

[tegefaulkes]

Specification

When calling cancel() on a Timer to clean it up, the underlying node timer is not cleared with clearTimeout() and the Timer promise never resolves or rejects. The prevents the process from ending until the timer delay has passed.

To reproduce this take the following steps.

1. Create a Timer with 10 seconds delay.
2. cancel the timer with any reason.
3. check if the timer rejects with provided reason immediately.
4. Check that the underlying node timer is cleaned up with clearTimeout(). This should be the case if the Timer rejected or resolved.
5. Check that the process is not held open after the timer is cancelled.

I haven't tested this problem in this code base yet. It seems odd that this would be a problem or hasn't been tested. There may be something weird going on.

Additional context

MatrixAI/Polykey#585

Tasks

1. Check if Timer is properly cleaning up when cancelled.
2. add tests checking this condition.
3. Apply any fix if needed.

[CMCDragonkai]

Should be a bug template not feature template.

[tegefaulkes]

This is an issue tracking the but and fix.

I've looked into this more, tests do test the proper behaviour when cancelling. But when we set lazy: true it breaks. lazy is only used in the reject method.

  protected async reject(reason?: any): Promise<void> {
    if (
      (this.lazy && (this._status == null || this._status === 'settling')) ||
      this._status === 'settled'
    ) {
      return;
    }
    this._status = 'settling';
    clearTimeout(this.timeoutRef);
    delete this.timeoutRef;
    this.rejectP(reason);
    this._status = 'settled';
  }

I think the intention here is if the handler is settling then we let it finish. BUT, if the timer has been created but hasn't timed out yet then the this._status is null. So if lazy is true, the timer is still running and cancel() is called then nothing happens. I think this is definitely a bug.

What I'd expect a lazy: true timer to do...

1. If cancelled when timer is still running then we reject with the reason and clean up`
2. If cancelled when timer has finished and the handler is running, it's up to the handler to deal with the signal.

diff --git a/src/Timer.ts b/src/Timer.ts
index f28d84d..8127a60 100644
--- a/src/Timer.ts
+++ b/src/Timer.ts
@@ -315,7 +315,7 @@ class Timer<T = void>
 
   protected async reject(reason?: any): Promise<void> {
     if (
-      (this.lazy && (this._status == null || this._status === 'settling')) ||
+      (this.lazy && this._status === 'settling') ||
       this._status === 'settled'
     ) {
       return;

It's a simple fix, but the latest version of this is using ESM now. So even if I fix staging I can't do a release the other repos can use.

[CMCDragonkai]

Do the fix by branching off when esm wasn't done and just do a fix there and release. I did the same for js-async-init.

[CMCDragonkai]

The purpose of the lazy is to avoid automatic rejection. To allow the handler to decide what to do.

[CMCDragonkai]

Yes if lazy is true and cancel is done, it should definitely cancel it. It's a bug. But I don't think the semantics of lazy should change.

[CMCDragonkai]

Branch off from ba97282 and release as 1.1.2 if you got a fix.

[tegefaulkes]

Fixed with the release of v1.1.2 c5525d9

[CMCDragonkai]

So now when you cancel a lazy timer, but it hasn't executed the handler yet, then the timer is immediately rejected with the cancellation reason.

We will need cherry-pick this feature into staging. Can you try that? I need to reopen until it's also part of mainline.

[CMCDragonkai]

Cherry pick only the change from feature-eager-cancellation, but not the release of 1.1.2, that will always live on in an orphaned branch.

[CMCDragonkai]

Cherry picked and worked e29b60e