Skip to content

Commit 70b7ff3

Browse files
sethvargoSAY-5
andauthored
fix: avoid Int63n panic on non-positive jitter (#39)
Both `WithJitter(0, ...)` and `WithJitterPercent(0, ...)` panic with "invalid argument to Int63n" because `rand.Int63n` requires a positive argument. Passing zero is a reasonable way to express "no jitter", and a computed jitter value can legitimately be zero. `WithJitter` additionally panics on a negative `time.Duration`. Both middlewares now short-circuit when the jitter argument is non-positive (`j <= 0`) and return the underlying backoff value unchanged. This builds on the original fix from @SAY-5 in #37, whose commit is preserved here for attribution. On top of it, this makes the `WithJitterPercent` guard `<= 0` to match `WithJitter`, and expands the regression tests into table-driven subtests covering zero jitter, a negative `WithJitter` argument (which would otherwise panic in `Int63n`), and stop propagation through the guard. --------- Signed-off-by: SAY-5 <say.apm35@gmail.com> Signed-off-by: Seth Vargo <seth@sethvargo.com> Co-authored-by: SAY-5 <say.apm35@gmail.com>
1 parent a324c68 commit 70b7ff3

2 files changed

Lines changed: 112 additions & 0 deletions

File tree

backoff.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ func WithJitter(j time.Duration, next Backoff) Backoff {
3434
return 0, true
3535
}
3636

37+
if j <= 0 {
38+
return val, false
39+
}
40+
3741
diff := time.Duration(r.Int63n(int64(j)*2) - int64(j))
3842
val = val + diff
3943
if val < 0 {
@@ -56,6 +60,10 @@ func WithJitterPercent(j uint64, next Backoff) Backoff {
5660
return 0, true
5761
}
5862

63+
if j <= 0 {
64+
return val, false
65+
}
66+
5967
// Get a value between -j and j, the convert to a percentage
6068
top := r.Int63n(int64(j)*2) - int64(j)
6169
pct := 1 - float64(top)/100.0

backoff_test.go

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,61 @@ func TestWithJitter(t *testing.T) {
5252
}
5353
}
5454

55+
func TestWithJitter_NonPositive(t *testing.T) {
56+
t.Parallel()
57+
58+
cases := []struct {
59+
name string
60+
j time.Duration
61+
nextVal time.Duration
62+
nextStop bool
63+
wantVal time.Duration
64+
wantStop bool
65+
}{
66+
{
67+
name: "zero",
68+
j: 0,
69+
nextVal: 1 * time.Second,
70+
wantVal: 1 * time.Second,
71+
},
72+
{
73+
name: "negative",
74+
j: -5 * time.Second,
75+
nextVal: 1 * time.Second,
76+
wantVal: 1 * time.Second,
77+
},
78+
{
79+
name: "stop_propagates",
80+
j: 0,
81+
nextStop: true,
82+
wantStop: true,
83+
},
84+
}
85+
86+
for _, tc := range cases {
87+
tc := tc
88+
89+
t.Run(tc.name, func(t *testing.T) {
90+
t.Parallel()
91+
92+
// A non-positive jitter must not panic (Int63n panics on a
93+
// non-positive argument) and must return the underlying value
94+
// unchanged.
95+
b := retry.WithJitter(tc.j, retry.BackoffFunc(func() (time.Duration, bool) {
96+
return tc.nextVal, tc.nextStop
97+
}))
98+
99+
val, stop := b.Next()
100+
if stop != tc.wantStop {
101+
t.Errorf("expected stop to be %t", tc.wantStop)
102+
}
103+
if val != tc.wantVal {
104+
t.Errorf("expected %v to be %v", val, tc.wantVal)
105+
}
106+
})
107+
}
108+
}
109+
55110
func ExampleWithJitter() {
56111
ctx := context.Background()
57112

@@ -84,6 +139,55 @@ func TestWithJitterPercent(t *testing.T) {
84139
}
85140
}
86141

142+
func TestWithJitterPercent_Zero(t *testing.T) {
143+
t.Parallel()
144+
145+
// j is a uint64, so zero is the only non-positive value it can hold.
146+
cases := []struct {
147+
name string
148+
j uint64
149+
nextVal time.Duration
150+
nextStop bool
151+
wantVal time.Duration
152+
wantStop bool
153+
}{
154+
{
155+
name: "zero",
156+
j: 0,
157+
nextVal: 1 * time.Second,
158+
wantVal: 1 * time.Second,
159+
},
160+
{
161+
name: "stop_propagates",
162+
j: 0,
163+
nextStop: true,
164+
wantStop: true,
165+
},
166+
}
167+
168+
for _, tc := range cases {
169+
tc := tc
170+
171+
t.Run(tc.name, func(t *testing.T) {
172+
t.Parallel()
173+
174+
// A zero jitter must not panic (Int63n panics on a non-positive
175+
// argument) and must return the underlying value unchanged.
176+
b := retry.WithJitterPercent(tc.j, retry.BackoffFunc(func() (time.Duration, bool) {
177+
return tc.nextVal, tc.nextStop
178+
}))
179+
180+
val, stop := b.Next()
181+
if stop != tc.wantStop {
182+
t.Errorf("expected stop to be %t", tc.wantStop)
183+
}
184+
if val != tc.wantVal {
185+
t.Errorf("expected %v to be %v", val, tc.wantVal)
186+
}
187+
})
188+
}
189+
}
190+
87191
func ExampleWithJitterPercent() {
88192
ctx := context.Background()
89193

0 commit comments

Comments
 (0)