Small refactor cancel context

[tinoquang]

Description

• Just a small change for checking canceled context. I moved the checking part into for loop to make sure context is always checked before calling f so if the context is canceled and it's not selected at the end of the loop, it will be checked one more time here
• Also updated benchmark_test to use the latest version of go-retry

[sethvargo]

Sadly, I think we actually need both. I think we need to keep this block and add it at the start of the for.

[sethvargo]

/cc @Deleplace

[Deleplace]

It's interesting because it's subtle and hard to reason about, even for such a relatively small amount of code.

The code on the left (before PR #11) checks ctx.Done():

• once before the loop, in case ctx was canceled before the 1st call to f
• once alone after f, in case ctx was canceled during the call to f (which can be slow)
• once in the delay step, in case ctx is canceled during the backoff pause

It is my understanding that ctx.Done() is already checked just before executing f, in all cases. At the end of the loop, the case where time.After "wins" is sufficient to proceed with looping and executing f.

Thus, I suggest to leave retry.go unchanged.

[tinoquang]

I think the behavior is still the same (please correct me if there's any special case). I'll explain each case:

once before the loop, in case ctx was canceled before the 1st call to f

• The check now is put before calling f in the loop so it will make sure that f will never execute if context was canceled

once alone after f, in case ctx was canceled during the call to f (which can be slow)

• It will be checked by the select block at the end of the loop

once in the delay step, in case ctx is canceled during the backoff pause

• This will also be checked by that select block. Here we have 3 cases:
  • If the context was canceled before, then ctx.Done() case is executed and the retry will stop
  • If the context is not canceled and the delay time is over then it will continue the loop --> check the context again (at the beginning of the loop)--> still not canceled --> execute f
  • In case there's a race between two conditions and time.After wins (because it's randomly chosen) then it will continue the loop --> check the context --> context is canceled --> return immediately

[Deleplace]

I do agree that the behaviour is the same.

So I'm changing my mind: in the light that this change in retry.go is not a bugfix, but a code style improvement, I think the change is good and may be merged.

[github-actions]

This pull request has been automatically locked since there has not
been any recent activity after it was closed. Please open a new
issue for related bugs.