Skip to content

Commit 6480241

Browse files
committed
fix: stop multipart producer after early response without race/hang
Create multipart cancel before the writer goroutine starts, and on transport return cancel + close pipe/readers so execute does not race on multipartCancelFunc or hang on multipartErrChan when a source Reader is blocked inside Read. Fixes #1186.
1 parent ea5b6cf commit 6480241

5 files changed

Lines changed: 161 additions & 7 deletions

File tree

client.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2485,9 +2485,12 @@ func (c *Client) execute(req *Request) (*Response, error) {
24852485

24862486
req.StartTime = time.Now()
24872487
resp, err := c.Client().Do(req.withTimeout())
2488-
// Cancel multipart context for io.Copy to stop reading/writing further
2489-
if req.isMultiPart && req.multipartCancelFunc != nil {
2490-
req.multipartCancelFunc()
2488+
// Stop multipart production once the transport has returned. Cancel alone
2489+
// is not enough when a source Reader is already blocked inside Read; also
2490+
// close closable field readers so the writer goroutine can finish and
2491+
// multipartErrChan can close (see #1186).
2492+
if req.isMultiPart {
2493+
req.stopMultipart()
24912494
}
24922495

24932496
// Take ownership of the per-attempt timeout cancel func set by
@@ -2509,8 +2512,14 @@ func (c *Client) execute(req *Request) (*Response, error) {
25092512
}
25102513
if req.isMultiPart && req.multipartErrChan != nil {
25112514
// read all multipart errors from channel
2512-
for err = range req.multipartErrChan {
2513-
response.CascadeError = wrapErrors(err, response.CascadeError)
2515+
for merr := range req.multipartErrChan {
2516+
// stopMultipart intentionally closes the pipe/readers after the
2517+
// transport returns. Those shutdown signals are not request failures
2518+
// when an HTTP response was already obtained.
2519+
if resp != nil && isMultipartStopError(merr) {
2520+
continue
2521+
}
2522+
response.CascadeError = wrapErrors(merr, response.CascadeError)
25142523
}
25152524
}
25162525

middleware.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,7 @@ func handleMultipart(c *Client, r *Request) error {
373373
br, bw := io.Pipe()
374374
mw := multipart.NewWriter(bw)
375375
r.Body = br
376+
r.multipartPipeWriter = bw
376377

377378
// set custom multipart boundary if exists
378379
if err := multipartSetBoundary(mw, r); err != nil {
@@ -382,6 +383,10 @@ func handleMultipart(c *Client, r *Request) error {
382383

383384
r.Header.Set(hdrContentTypeKey, mw.FormDataContentType())
384385

386+
// Create cancel before the writer goroutine so Client.execute can stop
387+
// production without racing on multipartCancelFunc publication.
388+
ctx, cancel := context.WithCancel(r.Context())
389+
r.multipartCancelFunc = cancel
385390
r.multipartErrChan = make(chan error, 1)
386391
go func() {
387392
defer close(r.multipartErrChan)
@@ -399,8 +404,6 @@ func handleMultipart(c *Client, r *Request) error {
399404
return
400405
}
401406

402-
ctx, cancel := context.WithCancel(r.Context())
403-
r.multipartCancelFunc = cancel
404407
for _, mf := range r.multipartFields {
405408
if mf.isValues() {
406409
for _, v := range mf.Values {

middleware_test.go

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"strings"
2222
"sync"
2323
"testing"
24+
"time"
2425
)
2526

2627
func Test_parseRequestURL(t *testing.T) {
@@ -1096,6 +1097,7 @@ func TestMiddleware_multipartWriteFormData(t *testing.T) {
10961097
})
10971098

10981099
req := &Request{
1100+
mu: new(sync.Mutex),
10991101
Header: http.Header{},
11001102
isMultiPart: true,
11011103
multipartFields: []*MultipartField{
@@ -1262,3 +1264,105 @@ func TestMiddlewareCoverage(t *testing.T) {
12621264
err1 := createRawRequest(c, req1)
12631265
assertTrue(t, strings.Contains(err1.Error(), "invalid character"), "invalid URL error expected")
12641266
}
1267+
1268+
func TestMultipartEarlyResponseRace(t *testing.T) {
1269+
client := NewWithClient(&http.Client{
1270+
Transport: earlyResponseTransport{},
1271+
})
1272+
1273+
for range 200 {
1274+
_, _ = client.R().
1275+
SetMultipartFields(&MultipartField{
1276+
Name: "audio",
1277+
FileName: "audio.wav",
1278+
ContentType: "audio/wav",
1279+
Reader: bytes.NewReader(make([]byte, 1<<20)),
1280+
}).
1281+
Post("http://resty.test/upload")
1282+
}
1283+
}
1284+
1285+
type earlyResponseTransport struct{}
1286+
1287+
func (earlyResponseTransport) RoundTrip(req *http.Request) (*http.Response, error) {
1288+
// net/http allows a RoundTripper to consume and close the request body
1289+
// asynchronously after RoundTrip returns.
1290+
go func() {
1291+
_, _ = io.Copy(io.Discard, req.Body)
1292+
_ = req.Body.Close()
1293+
}()
1294+
1295+
return &http.Response{
1296+
StatusCode: http.StatusUnprocessableEntity,
1297+
Status: "422 Unprocessable Entity",
1298+
Header: make(http.Header),
1299+
Body: io.NopCloser(strings.NewReader("rejected")),
1300+
Request: req,
1301+
}, nil
1302+
}
1303+
1304+
type blockingReadCloser struct {
1305+
started chan struct{}
1306+
release chan struct{}
1307+
once sync.Once
1308+
}
1309+
1310+
func newBlockingReadCloser() *blockingReadCloser {
1311+
return &blockingReadCloser{
1312+
started: make(chan struct{}),
1313+
release: make(chan struct{}),
1314+
}
1315+
}
1316+
1317+
func (r *blockingReadCloser) Read([]byte) (int, error) {
1318+
select {
1319+
case <-r.started:
1320+
default:
1321+
close(r.started)
1322+
}
1323+
1324+
<-r.release
1325+
1326+
return 0, io.EOF
1327+
}
1328+
1329+
func (r *blockingReadCloser) Close() error {
1330+
r.once.Do(func() {
1331+
close(r.release)
1332+
})
1333+
1334+
return nil
1335+
}
1336+
1337+
func TestMultipartReturnsAfterEarlyResponse(t *testing.T) {
1338+
// RoundTrip returns before the request body is fully written, leaving the
1339+
// multipart producer blocked inside Reader.Read. stopMultipart must cancel
1340+
// production and close closable field readers so execute does not hang on
1341+
// multipartErrChan (see #1186).
1342+
reader := newBlockingReadCloser()
1343+
client := NewWithClient(&http.Client{
1344+
Transport: earlyResponseTransport{},
1345+
})
1346+
1347+
done := make(chan error, 1)
1348+
go func() {
1349+
_, err := client.R().
1350+
SetMultipartFields(&MultipartField{
1351+
Name: "audio",
1352+
FileName: "audio.wav",
1353+
ContentType: "audio/wav",
1354+
Reader: reader,
1355+
}).
1356+
Post("http://resty.test/upload")
1357+
done <- err
1358+
}()
1359+
1360+
select {
1361+
case <-done:
1362+
// returned after early response without caller unblocking the reader
1363+
case <-time.After(2 * time.Second):
1364+
_ = reader.Close()
1365+
<-done
1366+
t.Fatal("multipart request did not return after the transport responded")
1367+
}
1368+
}

request.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ type Request struct {
108108
unescapeQueryParams bool
109109
multipartErrChan chan error
110110
multipartCancelFunc context.CancelFunc
111+
multipartPipeWriter *io.PipeWriter
111112
}
112113

113114
// SetCorrelationID method is used to set the correlation ID for the request
@@ -1597,6 +1598,26 @@ func (r *Request) Execute(method, url string) (res *Response, err error) {
15971598
return
15981599
}
15991600

1601+
// stopMultipart cancels multipart streaming, closes the pipe writer, and closes
1602+
// field readers so a writer blocked in io.Reader.Read can unblock once the HTTP
1603+
// round-trip ends (or needs to be aborted).
1604+
func (r *Request) stopMultipart() {
1605+
if r == nil {
1606+
return
1607+
}
1608+
if r.multipartCancelFunc != nil {
1609+
r.multipartCancelFunc()
1610+
}
1611+
if r.multipartPipeWriter != nil {
1612+
// Unblock Client.Do / RoundTrip waiting on the request body pipe.
1613+
_ = r.multipartPipeWriter.CloseWithError(io.ErrClosedPipe)
1614+
r.multipartPipeWriter = nil
1615+
}
1616+
for _, mf := range r.multipartFields {
1617+
mf.close()
1618+
}
1619+
}
1620+
16001621
// Clone returns a deep copy of r with its context changed to ctx.
16011622
// It does clone appropriate fields, reset, and reinitialize, so
16021623
// [Request] can be used again.
@@ -1662,6 +1683,8 @@ func (r *Request) Clone(ctx context.Context) *Request {
16621683
rr.initTraceIfEnabled()
16631684
rr.values = make(map[string]any)
16641685
rr.multipartErrChan = nil
1686+
rr.multipartCancelFunc = nil
1687+
rr.multipartPipeWriter = nil
16651688
rr.ctxCancelFunc = nil
16661689

16671690
// copy bodyBuf

util.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package resty
77

88
import (
99
"bytes"
10+
"context"
1011
"crypto/rand"
1112
"crypto/sha256"
1213
"encoding/binary"
@@ -545,3 +546,17 @@ func readMachineID() []byte {
545546
// This panic would happen at program startup, so no worries at runtime panic.
546547
panic(errors.New("resty - guid: unable to get hostname and random bytes"))
547548
}
549+
550+
func isMultipartStopError(err error) bool {
551+
if err == nil {
552+
return false
553+
}
554+
if errors.Is(err, io.ErrClosedPipe) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
555+
return true
556+
}
557+
// multipart.Writer.Close can surface closed-pipe as a plain error string
558+
// depending on Go version / close path.
559+
msg := err.Error()
560+
return strings.Contains(msg, "io: read/write on closed pipe") ||
561+
strings.Contains(msg, "context canceled")
562+
}

0 commit comments

Comments
 (0)