-
Notifications
You must be signed in to change notification settings - Fork 203
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
[Fake clock] Make Stop / Reset return false if Timer stopped #320
[Fake clock] Make Stop / Reset return false if Timer stopped #320
Conversation
In order to conform to the standard library Timer, Stop and Reset should return false if the Timer has already been stopped, which was not the case previously. We add unit tests to validate the new implementation. The updated TestFakeStop test case and the new TestFakeReset/reset_stopped_timer case fail with the old implementation. We also simplify the implementation of Stop and Rest in the following way: there is no need to check f.waiter.fired to determine whether a Timer has expired. For Timers, this flag is set atomically with the waiter being removed from the waiters list. As a result, we only check for absence of the waiter from the list, which either indicates that the Timer has been stopped or that it has already expired (fired). Signed-off-by: Antonin Bas <[email protected]>
Welcome @antoninbas! |
thanks for the PR, will defer review to others, but opened kubernetes/kubernetes#128944 in the meantime to make sure unit tests in kubernetes/kubernetes still pass with this change |
Thanks @liggitt - I am happy to help look into any test failures that may arise |
@liggitt it seems that the 2 test failures for your validation PR are related to dependencies, and may be expected in this case? I can see that |
/assign @MadhavJivrajani @thockin if you want to take a peek, it looks good to me and the change works fine in k/k's unit tests as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change LGTM, but I think can be cleaned up further - agree or did I miss something?
@@ -198,12 +198,10 @@ func (f *FakeClock) setTimeLocked(t time.Time) { | |||
if w.skipIfBlocked { | |||
select { | |||
case w.destChan <- t: | |||
w.fired = true | |||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is still defined in the struct, L51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, sorry about that
removed
Removed unused fired field for waiters. Signed-off-by: Antonin Bas <[email protected]>
e00eb21
to
89f8b1e
Compare
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: antoninbas, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks for the careful review @thockin ! |
What type of PR is this?
This is a bug in the fake clock implementation, but not one that impacts K8s users.
What this PR does / why we need it:
In order to conform to the standard library Timer, Stop and Reset should return false if the Timer has already been stopped, which was not the case previously.
We add unit tests to validate the new implementation. The updated TestFakeStop test case and the new TestFakeReset/reset_stopped_timer case fail with the old implementation.
We also simplify the implementation of Stop and Rest in the following way: there is no need to check f.waiter.fired to determine whether a Timer has expired. For Timers, this flag is set atomically with the waiter being removed from the waiters list. As a result, we only check for absence of the waiter from the list, which either indicates that the Timer has been stopped or that it has already expired (fired).
Which issue(s) this PR fixes:
Special notes for your reviewer:
Release note: