Skip to content

Commit b17d8b0

Browse files
authored
fix: context cancellation leak (#1177)
On the success path the request-level timeout context's cancel func is never called, so it leaks
1 parent b5060b2 commit b17d8b0

5 files changed

Lines changed: 181 additions & 4 deletions

File tree

client.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2490,10 +2490,21 @@ func (c *Client) execute(req *Request) (*Response, error) {
24902490
req.multipartCancelFunc()
24912491
}
24922492

2493+
// Take ownership of the per-attempt timeout cancel func set by
2494+
// withTimeout. It must fire once the response body is fully consumed,
2495+
// so it is attached to the body's Close below. On a transport error
2496+
// there is no body to read, so release it right away to avoid leaking
2497+
// the context (and its timer) until the deadline elapses.
2498+
cancel := req.ctxCancelFunc
2499+
req.ctxCancelFunc = nil
2500+
24932501
response := &Response{Request: req, RawResponse: resp}
24942502
response.setReceivedAt()
24952503
if err != nil {
24962504
c.cbRequestError()
2505+
if cancel != nil {
2506+
cancel()
2507+
}
24972508
return response, err
24982509
}
24992510
if req.isMultiPart && req.multipartErrChan != nil {
@@ -2509,6 +2520,14 @@ func (c *Client) execute(req *Request) (*Response, error) {
25092520
}
25102521

25112522
response.Body = resp.Body
2523+
// Release the request timeout context once the body is closed,
2524+
// whether by the caller (do-not-parse) or by Resty while parsing
2525+
// or draining the body. Wrapping innermost ensures cancel runs no
2526+
// matter which outer reader closes the chain.
2527+
if cancel != nil {
2528+
response.Body = &cancelReadCloser{r: response.Body, cancel: cancel}
2529+
cancel = nil
2530+
}
25122531
if err = response.wrapContentDecompresser(); err != nil {
25132532
return response, response.wrapError(err, false)
25142533
}
@@ -2526,6 +2545,12 @@ func (c *Client) execute(req *Request) (*Response, error) {
25262545
}
25272546
}
25282547

2548+
// No response body was available to attach the cancel to (e.g. resp is
2549+
// nil); release the timeout context now so it does not leak.
2550+
if cancel != nil {
2551+
cancel()
2552+
}
2553+
25292554
debugLogger(c, response)
25302555

25312556
// Apply Response middleware

context_test.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,85 @@ func TestSSESourceSetContextCancel(t *testing.T) {
317317
}
318318
}
319319

320+
func TestRequestTimeoutContextReleasedAfterBodyRead(t *testing.T) {
321+
ts := createTestServer(func(w http.ResponseWriter, r *http.Request) {
322+
_, _ = w.Write([]byte("ok"))
323+
})
324+
defer ts.Close()
325+
326+
tr := &ctxCaptureTransport{rt: http.DefaultTransport}
327+
c := dcnl().SetTransport(tr)
328+
329+
resp, err := c.R().
330+
SetTimeout(5 * time.Second).
331+
Get(ts.URL + "/")
332+
333+
assertNil(t, err)
334+
_ = resp.String() // reads and closes the body
335+
336+
assertNotNil(t, tr.ctx, "expected transport to have captured a context")
337+
assertNotNil(t, tr.ctx.Err(), "expected timeout context to be cancelled after body is closed")
338+
}
339+
340+
func TestRequestTimeoutContextReleasedOnTransportError(t *testing.T) {
341+
transportErr := errors.New("simulated transport error")
342+
tr := &ctxCaptureTransport{
343+
rt: roundTripFunc(func(r *http.Request) (*http.Response, error) {
344+
return nil, transportErr
345+
}),
346+
}
347+
c := dcnl().SetTransport(tr)
348+
349+
_, err := c.R().
350+
SetTimeout(5 * time.Second).
351+
Get("http://127.0.0.1:1/unreachable")
352+
353+
assertNotNil(t, err)
354+
assertNotNil(t, tr.ctx, "expected transport to have captured a context")
355+
assertNotNil(t, tr.ctx.Err(), "expected timeout context to be cancelled after transport error")
356+
}
357+
358+
func TestRequestTimeoutContextReleasedOnDoNotParseResponse(t *testing.T) {
359+
ts := createTestServer(func(w http.ResponseWriter, r *http.Request) {
360+
_, _ = w.Write([]byte("ok"))
361+
})
362+
defer ts.Close()
363+
364+
tr := &ctxCaptureTransport{rt: http.DefaultTransport}
365+
c := dcnl().SetTransport(tr)
366+
367+
resp, err := c.R().
368+
SetTimeout(5 * time.Second).
369+
SetResponseDoNotParse(true).
370+
Get(ts.URL + "/")
371+
372+
assertNil(t, err)
373+
assertNotNil(t, resp.Body, "expected response body to be non-nil")
374+
375+
// Before closing, context should still be live.
376+
assertNil(t, tr.ctx.Err(), "expected timeout context to still be active before body is closed")
377+
378+
_ = resp.Body.Close()
379+
380+
assertNotNil(t, tr.ctx.Err(), "expected timeout context to be cancelled after body is closed")
381+
}
382+
383+
// ctxCaptureTransport records the context of the outgoing request.
384+
type ctxCaptureTransport struct {
385+
rt http.RoundTripper
386+
ctx context.Context
387+
}
388+
389+
func (t *ctxCaptureTransport) RoundTrip(r *http.Request) (*http.Response, error) {
390+
t.ctx = r.Context()
391+
return t.rt.RoundTrip(r)
392+
}
393+
394+
// roundTripFunc is an http.RoundTripper backed by a plain function.
395+
type roundTripFunc func(*http.Request) (*http.Response, error)
396+
397+
func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
398+
320399
func errIsContextCanceled(err error) bool {
321400
return errors.Is(err, context.Canceled)
322401
}

request.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1501,11 +1501,10 @@ func (r *Request) Execute(method, url string) (res *Response, err error) {
15011501
isInvalidRequestErr = true
15021502
break
15031503
}
1504+
// The per-attempt timeout context cancel func is owned and
1505+
// released by Client.execute (on transport error, or when the
1506+
// response body is closed), so there is nothing to cancel here.
15041507
if r.Context().Err() != nil {
1505-
if r.ctxCancelFunc != nil {
1506-
r.ctxCancelFunc()
1507-
r.ctxCancelFunc = nil
1508-
}
15091508
if !errors.Is(err, context.DeadlineExceeded) {
15101509
err = wrapErrors(r.Context().Err(), err)
15111510
break

stream.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,33 @@ func (r *copyReadCloser) Close() error {
378378
return nil
379379
}
380380

381+
var _ io.ReadCloser = (*cancelReadCloser)(nil)
382+
383+
// cancelReadCloser wraps the response body so that closing it also invokes
384+
// cancel, releasing the per-request timeout context created by
385+
// [Request.withTimeout].
386+
//
387+
// The cancel must not run before the body is fully consumed; otherwise an
388+
// in-flight body read would be aborted with a context error. Closing the body
389+
// (directly by the caller for do-not-parse responses, or by Resty while
390+
// parsing/draining otherwise) is therefore the correct moment to release it.
391+
// context cancel funcs are safe to call more than once, so repeated Close
392+
// calls are harmless.
393+
type cancelReadCloser struct {
394+
r io.ReadCloser
395+
cancel context.CancelFunc
396+
}
397+
398+
func (c *cancelReadCloser) Read(p []byte) (int, error) {
399+
return c.r.Read(p)
400+
}
401+
402+
func (c *cancelReadCloser) Close() error {
403+
err := c.r.Close()
404+
c.cancel()
405+
return err
406+
}
407+
381408
var _ io.ReadCloser = (*nopReadCloser)(nil)
382409

383410
type nopReadCloser struct {

stream_test.go

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"compress/flate"
66
"compress/gzip"
7+
"errors"
78
"io"
89
"net/http"
910
"net/http/httptest"
@@ -598,6 +599,52 @@ func TestDecodeXML(t *testing.T) {
598599
})
599600
}
600601

602+
func TestCancelReadCloser(t *testing.T) {
603+
t.Run("read delegates to inner reader", func(t *testing.T) {
604+
data := []byte("hello resty")
605+
rc := &cancelReadCloser{
606+
r: io.NopCloser(bytes.NewReader(data)),
607+
cancel: func() {},
608+
}
609+
buf := make([]byte, len(data))
610+
n, err := rc.Read(buf)
611+
assertNil(t, err)
612+
assertEqual(t, len(data), n)
613+
assertEqual(t, string(data), string(buf))
614+
})
615+
616+
t.Run("close calls cancel", func(t *testing.T) {
617+
canceled := false
618+
rc := &cancelReadCloser{
619+
r: io.NopCloser(strings.NewReader("")),
620+
cancel: func() { canceled = true },
621+
}
622+
err := rc.Close()
623+
assertNil(t, err)
624+
assertTrue(t, canceled, "expected cancel to be called on Close")
625+
})
626+
627+
t.Run("close returns inner error", func(t *testing.T) {
628+
closeErr := errors.New("inner close error")
629+
canceled := false
630+
rc := &cancelReadCloser{
631+
r: &errReadCloser{closeErr: closeErr},
632+
cancel: func() { canceled = true },
633+
}
634+
err := rc.Close()
635+
assertEqual(t, closeErr, err)
636+
assertTrue(t, canceled, "expected cancel to be called even when inner Close errors")
637+
})
638+
}
639+
640+
// errReadCloser is a ReadCloser whose Close returns a fixed error.
641+
type errReadCloser struct {
642+
closeErr error
643+
}
644+
645+
func (e *errReadCloser) Read(p []byte) (int, error) { return 0, io.EOF }
646+
func (e *errReadCloser) Close() error { return e.closeErr }
647+
601648
func TestStreamMisc(t *testing.T) {
602649
t.Run("wrapper gzip reader is nil", func(t *testing.T) {
603650
// Simulate a scenario where gzip.NewReader returns a wrapper with nil gr

0 commit comments

Comments
 (0)