fix: Do first try immediately, then activate floor #27
Conversation
There was a problem hiding this comment.
While this change seems innocuous enough, I could imagine an existing use case similar to the following:
var wg sync.WaitGroup
for i := 0; i < 1_000_000; i++ {
wg.Add(1)
go func() {
defer wg.Done()
for retrier := retry.New(rand.Intn(30) * time.Second, time.Minute); retrier.Wait(ctx); {
if err := doSomething(); err == nil {
return
}
}
}
}
wg.Wait()
With the current behaviour, the parallel executions of doSomething would be smeared across the initial delay. The new behaviour would cause a thundering herd of doSomethings() all at once, which would be a rather unexpected change IMO.
|
@johnstcn that's a great observation and definitely worth consideration. I don't know what the original intent here was, but to me it feels weird that when using this library you'd have to write it like this: if err := doSomething(); err == nil {
return
}
for r := retry.New(time.Second, 10*time.Second); r.Wait(ctx); {
if err := doSomething(); err == nil {
return
}
}Our current use in https://github.com/coder/coder all seems to behave in such a way that I would expect the behavior provided by this PR, but |
A quick search on SourceGraph doesn't bring up any public external usages. That doesn't rule out private repo usage however. |
|
Judging by the old API, https://github.com/coder/retry/blob/v1.2.0/retry.go#L274-L288, I'd say this is how this library used to behave as well. I.e. first try is done immediately, then sleep. |
|
The current api matches how https://goplay.tools/snippet/C_LwHjojQi9 Food for thought. |
|
just noticed the rety package we use in v1 is totally different syntax we use a different version of the same package, and the package looks entirely different 🤷♂️ |
|
@Emyrk fair point, but do you think people would expect it to behave the same way? The only way I can make sense of the API as it's intended to be used (readme), and enforcing an initial delay, would be to have an explicit argument for it. 🤔 |
|
@mafredri I think you are right, it should fire immediately. Was just trying to play devil's advocate in any rational of the opposite 😄. I then realized then the You could always make it some argument in the chaining if someone needs that original delay. tl'dr I like your PR, I just do not know the impact |
I noticed retries were always taking
floorto activate (https://github.com/coder/coder/blob/af59e2bcfa524dd0f93a186b69a8e43d2a31702e/agent/agent.go#L189), and then realized that floor isn't really even used here. So I assumed this was a bug and changed it so that first try is done immediately.