Don't leak timers #99
Conversation
When in the rollback loop, a timer was created, but if the context is finished, then the timer's channel would leak. In addition, the function said it was doing exponential backoff (presumably from 0 to 512 seconds), but the arguments to the `Exp()` function were reversed, so it was doing quadratic backoff from 0 to 81 seconds. Also, there was no test for the rollback function, so one was added.
| var ( | ||
| rollbackAttempts = 10 | ||
| minRollbackDuration = 1 * time.Second | ||
| maxRollbackDuration = 100 * time.Second |
There was a problem hiding this comment.
curious how we decided on the value for what a good max duration would be
There was a problem hiding this comment.
I think those are based on the previous backoff implementation.
There was a problem hiding this comment.
Yep, I didn't want to go significantly higher than the previous max, which was 81 seconds.
| for i := 0; i < 10; i++ { | ||
| waitSeconds := math.Pow(float64(i), 2) | ||
| timer := time.NewTimer(time.Duration(waitSeconds) * time.Second) | ||
| for { |
There was a problem hiding this comment.
I like the update to use thebackoff package here!
| var ( | ||
| rollbackAttempts = 10 | ||
| minRollbackDuration = 1 * time.Second | ||
| maxRollbackDuration = 100 * time.Second |
There was a problem hiding this comment.
I think those are based on the previous backoff implementation.
path_rotate.go
Outdated
| @@ -181,26 +187,32 @@ func (b *backend) pathRotateRoleCredentialsUpdate(ctx context.Context, req *logi | |||
| return nil, nil | |||
| } | |||
|
|
|||
| // rollBackPassword uses naive exponential backoff to retry updating to an old password, | |||
| // rollBackPassword uses exponential backoff to retry updating to an old password, | |||
There was a problem hiding this comment.
nit: suggest renaming to rollbackPassword
path_rotate_test.go
Outdated
| fclient.password = "password" | ||
| err := b.rollBackPassword(ctx, cfg, "old") | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, "old", fclient.password) |
There was a problem hiding this comment.
Can we also assert something about fclient.count?
path_rotate_test.go
Outdated
|
|
||
| ctx := context.Background() | ||
|
|
||
| // works if the client always succeeds |
There was a problem hiding this comment.
I love the tests here! I think converting this to be a table driven test might be a cleaner approach. We could get a fake client for each sub test.
path_rotate_test.go
Outdated
|
|
||
| var _ ldapClient = (*failingRollbackClient)(nil) | ||
|
|
||
| func TestRollback(t *testing.T) { |
There was a problem hiding this comment.
nit: consider renaming to Test_rollbackPassword or TestRollbackPassword to match the name of the function being tested here.
Co-authored-by: Ben Ash <[email protected]>
|
Thanks! |
When in the rollback loop, a timer was created, but if the context is finished, then the timer's channel would leak.
In addition, the function said it was doing exponential backoff (presumably from 0 to 512 seconds), but the arguments to the
Pow()function were reversed, so it was doing quadratic backoff from 0 to 81 seconds.Also, there was no test for the rollback function, so one was added.