Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support timer refresh and reset #5

Closed
2 tasks done
tegefaulkes opened this issue Mar 14, 2023 · 13 comments · Fixed by #6
Closed
2 tasks done

feat: support timer refresh and reset #5

tegefaulkes opened this issue Mar 14, 2023 · 13 comments · Fixed by #6
Assignees
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Mar 14, 2023

Specification

To complete MatrixAI/Polykey#513 we need to add two features to the Timer.

  1. The ability to refresh the timer to timeout from the current time with the existing delay.
  2. The ability to reset the timer to timeout from the current time with a new delay.

Both cases need to update the start time and end time of the timer with reset updating the delay as well.

Additional context

Tasks

  1. add refresh method to the Timer.
  2. add reset method to the Timer.
@tegefaulkes tegefaulkes added the development Standard development label Mar 14, 2023
@tegefaulkes tegefaulkes self-assigned this Mar 14, 2023
@tegefaulkes
Copy link
Contributor Author

There is one thing I'm not sure about. The timestamp is set to the date of when the timer was created. Should we update this to the current time when the timer is refreshed or reset? Do we separate it into a createTime and startTime?
Or do we not bother changing it at all since start time can be back-calculated as scheduleTime-delay?

@tegefaulkes
Copy link
Contributor Author

Actually, it's not worth updating the timestamp, We just need to update the schedule time.

@CMCDragonkai
Copy link
Member

As a side note make sure that once the handler is executed, make sure that those methods throws an exception.

@tegefaulkes
Copy link
Contributor Author

I've made them a NOP, do they really need to throw an exception?

@tegefaulkes
Copy link
Contributor Author

I can add in errors but since we don't have any errors in this repo already I have some questions.

  1. Do we want to replicate the error style from Polykey? Where we have a base error that we extend from?
  2. Do I add in the sysexit codes?
  3. Is it worth adding all this in for a single error? Could I just use a simple error of Error('Timer ended')?

@CMCDragonkai
Copy link
Member

Please replicate the error style we are using in all other libraries like EFS and Async-init... Etc.

@CMCDragonkai
Copy link
Member

So there should be an errors.ts at the src. Consistency is important here. You may need to bring in js-errors.

@CMCDragonkai
Copy link
Member

I've made them a NOP, do they really need to throw an exception?

Yes for intended behaviour.

@CMCDragonkai CMCDragonkai added the r&d:polykey:supporting activity Supporting core activity label Jul 9, 2023
@CMCDragonkai
Copy link
Member

CMCDragonkai commented Sep 10, 2023

This:

    this.timeoutRef.refresh();

Does not exist in browsers.

We should eventually want to make this work in browsers, so this should be changed.

@CMCDragonkai
Copy link
Member

Instead of checking if the timeoutRef is null which isn't really guaranteed, only because I ran delete this.timeoutRef. The more correct option is to check this._status is not equal to null. Which ensures that you cannot call t.reset(...) or t.refresh() if the timer is settling or settled. Which is more obvious.

@CMCDragonkai
Copy link
Member

I fixed both of the above in 5624c4f

@CMCDragonkai
Copy link
Member

Also as I was working in MatrixAI/js-quic#53, I noticed some complicated cancellation of timers. So I added tests to prove how cancel works when the timer is in the midst of settling or settled. The behaviour is now more obvious.

  1. If a eager timer (default) is settling - cancelling a timer rejects the timer promise, and the handler signal abortion is true. The handler can continue executing though.
  2. If a lazy timer is settling - cancelling a timer sets the handler signal abortion to true. Whether it resolves or rejects depends on the handler.
  3. If a timer lazy or eager is settled, cancelling has no effect.

Thus cancelling timers like keep alive timer and conn timeout timer is really simple:

this.xTimer?.cancel();

Is sufficient.

At the same time, the handler should then be running and checking if the abortion signal is true. If so, it should stop what it is doing. Otherwise it should propagate the cancellation to any underlying promises.

@CMCDragonkai
Copy link
Member

A side effect of 5624c4f is that it also fixes the case where the timer is an infinite timer, and you call t.refresh(), in that case, it essentially does nothing. Because refreshing an infinite timer is basically a noop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity
Development

Successfully merging a pull request may close this issue.

2 participants