Skip to content

Commit a7bf63c

Browse files
committed
fix(retry): implement two-stage select to prioritize context cancellation
- Add non-blocking select stage to check context/closer before timer - Remove skipUnderDuress logic from tests as no longer needed - Revert timing tolerance changes since root cause is fixed - Eliminates race condition in retry.Next() select statement Implements solution suggested by @kev-cao in #154764
1 parent 631fc54 commit a7bf63c

File tree

2 files changed

+11
-13
lines changed

2 files changed

+11
-13
lines changed

pkg/util/retry/retry.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,15 @@ func (r *Retry) Next() bool {
192192
r.backingOffHook(actualWait)
193193
}
194194

195+
// Check for cancellation first to prioritize over timer.
196+
select {
197+
case <-r.opts.Closer:
198+
return false
199+
case <-r.ctx.Done():
200+
return false
201+
default:
202+
}
203+
195204
select {
196205
case <-r.opts.Closer:
197206
return false

pkg/util/retry/retry_test.go

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,6 @@ func TestRetryWithMaxDuration(t *testing.T) {
406406
// an upper bound instead (e.g. for context or closer).
407407
maxExpectedTimeSpent time.Duration
408408
expectedErr bool
409-
skipUnderDuress bool
410409
}
411410

412411
testCases := []testcase{
@@ -474,11 +473,8 @@ func TestRetryWithMaxDuration(t *testing.T) {
474473
preRetryFunc: func() {
475474
cancelCtxFunc()
476475
},
477-
maxExpectedTimeSpent: time.Millisecond * 30,
476+
maxExpectedTimeSpent: time.Millisecond * 20,
478477
expectedErr: true,
479-
// Under duress, closing a context will not necessarily stop the retry
480-
// loop immediately, so we skip this test under duress.
481-
skipUnderDuress: true,
482478
},
483479
{
484480
name: "errors with opt.Closer that is closed",
@@ -494,20 +490,13 @@ func TestRetryWithMaxDuration(t *testing.T) {
494490
preRetryFunc: func() {
495491
close(closeCh)
496492
},
497-
maxExpectedTimeSpent: time.Millisecond * 30,
493+
maxExpectedTimeSpent: time.Millisecond * 20,
498494
expectedErr: true,
499-
// Under duress, closing a channel will not necessarily stop the retry
500-
// loop immediately, so we skip this test under duress.
501-
skipUnderDuress: true,
502495
},
503496
}
504497

505498
for _, tc := range testCases {
506499
t.Run(tc.name, func(t *testing.T) {
507-
if tc.skipUnderDuress && skip.Duress() {
508-
skip.UnderDuress(t, "skipping test under duress: %s", tc.name)
509-
}
510-
511500
timeSource := timeutil.NewManualTime(time.Now())
512501
tc.opts.Clock = timeSource
513502
// Disable randomization for deterministic tests.

0 commit comments

Comments
 (0)