Skip to content

Commit fef6765

Browse files
mruegclaude
andcommitted
fix: guard the debug-log type assertion and keep the rate limiter's context cause
Two small robustness fixes. 1. debugLogger asserted an interface value unconditionally. Request: req.values[debugRequestLogKey].(*DebugLogRequest), prepareRequestDebugInfo populates that key before the request is sent, so the key is normally present -- but if it is not, this panics inside a logging path: panic: interface conversion: interface {} is nil, not *resty.DebugLogRequest Now guarded with the two-value form and an empty fallback, so debug logging can degrade rather than take the process down. 2. Rate limiter errors erased the context cause. Both RateLimitTokenBucket.Allow and RateLimitSlidingWindow.Allow returned the bare ErrRateLimitExceeded sentinel whether the wait ended in cancellation or a deadline, so callers could not tell the two apart and errors.Is(err, context.Canceled) never matched. The context error is now wrapped alongside the sentinel, so both checks succeed: errors.Is(err, resty.ErrRateLimitExceeded) // still true errors.Is(err, context.DeadlineExceeded) // now also true NOTE: this is a behaviour change for anyone comparing with `err == resty.ErrRateLimitExceeded`; errors.Is is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 503cee1 commit fef6765

4 files changed

Lines changed: 80 additions & 5 deletions

File tree

debug.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,15 @@ func debugLogger(c *Client, res *Response) {
137137
Body: res.fmtBodyString(res.Request.DebugBodyLimit),
138138
}
139139

140+
// prepareRequestDebugInfo populates this before the request is sent. Guard the
141+
// assertion anyway so a logging path can never panic.
142+
rql, _ := req.values[debugRequestLogKey].(*DebugLogRequest)
143+
if rql == nil {
144+
rql = &DebugLogRequest{}
145+
}
146+
140147
dl := &DebugLog{
141-
Request: req.values[debugRequestLogKey].(*DebugLogRequest),
148+
Request: rql,
142149
Response: rdl,
143150
}
144151

rate_limiter.go

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ package resty
88
import (
99
"context"
1010
"errors"
11+
"fmt"
1112
"sync"
1213
"time"
1314
)
@@ -16,8 +17,23 @@ import (
1617
// rejects a request. This occurs when the context is cancelled or the deadline
1718
// expires before a token becomes available, or immediately if the rate limiter
1819
// implementation rejects the request for any other reason.
20+
//
21+
// The limiters shipped with Resty wrap the context error alongside this sentinel,
22+
// so both [errors.Is] checks succeed:
23+
//
24+
// errors.Is(err, resty.ErrRateLimitExceeded)
25+
// errors.Is(err, context.DeadlineExceeded)
1926
var ErrRateLimitExceeded = errors.New("resty: rate limit exceeded")
2027

28+
// rateLimitError joins ErrRateLimitExceeded with the context error that caused
29+
// the wait to be abandoned, so callers can tell cancellation from a deadline.
30+
func rateLimitError(ctx context.Context) error {
31+
if err := ctx.Err(); err != nil {
32+
return fmt.Errorf("%w: %w", ErrRateLimitExceeded, err)
33+
}
34+
return ErrRateLimitExceeded
35+
}
36+
2137
// RateLimiter is the interface that wraps the rate limiting behavior used by
2238
// [Client]. Implement this interface to provide custom rate limiting strategies.
2339
// The [Client] calls [RateLimiter.Allow] before every request; if it returns
@@ -117,7 +133,7 @@ func (l *RateLimitTokenBucket) Allow(ctx context.Context) error {
117133
// Check context first to avoid acquiring the lock unnecessarily.
118134
select {
119135
case <-ctx.Done():
120-
return ErrRateLimitExceeded
136+
return rateLimitError(ctx)
121137
default:
122138
}
123139

@@ -138,7 +154,7 @@ func (l *RateLimitTokenBucket) Allow(ctx context.Context) error {
138154
select {
139155
case <-ctx.Done():
140156
timer.Stop()
141-
return ErrRateLimitExceeded
157+
return rateLimitError(ctx)
142158
case <-timer.C:
143159
}
144160
}
@@ -232,7 +248,7 @@ func (l *RateLimitSlidingWindow) Allow(ctx context.Context) error {
232248
for {
233249
select {
234250
case <-ctx.Done():
235-
return ErrRateLimitExceeded
251+
return rateLimitError(ctx)
236252
default:
237253
}
238254

@@ -268,7 +284,7 @@ func (l *RateLimitSlidingWindow) Allow(ctx context.Context) error {
268284
select {
269285
case <-ctx.Done():
270286
timer.Stop()
271-
return ErrRateLimitExceeded
287+
return rateLimitError(ctx)
272288
case <-timer.C:
273289
}
274290
}

rate_limiter_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,3 +252,34 @@ func TestRateLimiterSlidingWindowConfig(t *testing.T) {
252252
assertEqual(t, time.Second, l.WindowSize(), "expected default window of 1s")
253253
})
254254
}
255+
256+
// Both limiters collapsed cancellation and deadline expiry into the bare
257+
// sentinel, so callers could not tell them apart.
258+
func TestRateLimiterErrorWrapsContextCause(t *testing.T) {
259+
limiters := map[string]RateLimiter{
260+
"token bucket": NewRateLimitTokenBucket(1, 1),
261+
"sliding window": NewRateLimitSlidingWindow(1, time.Hour),
262+
}
263+
264+
for name, l := range limiters {
265+
t.Run(name, func(t *testing.T) {
266+
assertNil(t, l.Allow(context.Background())) // consume the only slot
267+
268+
t.Run("cancelled", func(t *testing.T) {
269+
ctx, cancel := context.WithCancel(context.Background())
270+
cancel()
271+
err := l.Allow(ctx)
272+
assertErrorIs(t, ErrRateLimitExceeded, err)
273+
assertErrorIs(t, context.Canceled, err)
274+
})
275+
276+
t.Run("deadline exceeded", func(t *testing.T) {
277+
ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond)
278+
defer cancel()
279+
err := l.Allow(ctx)
280+
assertErrorIs(t, ErrRateLimitExceeded, err)
281+
assertErrorIs(t, context.DeadlineExceeded, err)
282+
})
283+
})
284+
}
285+
}

resty_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"errors"
1919
"fmt"
2020
"io"
21+
"log"
2122
"net"
2223
"net/http"
2324
"net/http/httptest"
@@ -1077,3 +1078,23 @@ func createBinFile(fileName string, size int64) string {
10771078
_ = f.Close()
10781079
return fp
10791080
}
1081+
1082+
// debugLogger asserted req.values[debugRequestLogKey] unconditionally, so a
1083+
// debug-enabled request whose values map has no entry panicked in a logging path.
1084+
func TestDebugLoggerWithoutPreparedRequestLog(t *testing.T) {
1085+
var logBuf bytes.Buffer
1086+
c := New().SetLogger(&logger{l: log.New(&logBuf, "", 0)})
1087+
defer c.Close()
1088+
1089+
req := c.R()
1090+
req.IsDebug = true
1091+
req.initValuesMap() // deliberately empty: no debugRequestLogKey
1092+
1093+
res := &Response{Request: req}
1094+
res.setReceivedAt()
1095+
1096+
debugLogger(c, res) // must not panic
1097+
1098+
assertTrue(t, strings.Contains(logBuf.String(), "RESPONSE"),
1099+
"expected the debug log to be written, got: "+logBuf.String())
1100+
}

0 commit comments

Comments
 (0)