From 5d5fd985ebf726514e40c79862921c39e4c3b41c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=9F=E5=A4=A7=E9=99=86?= <10188142zhong_dalu@cn.tre-inc.com> Date: Tue, 21 Mar 2023 13:52:40 +0800 Subject: [PATCH 1/4] fix sync pool data race: deep copy --- middleware.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/middleware.go b/middleware.go index 537a6286..712c32c5 100644 --- a/middleware.go +++ b/middleware.go @@ -169,7 +169,9 @@ func createHTTPRequest(c *Client, r *Request) (err error) { r.RawRequest, err = http.NewRequest(r.Method, r.URL, nil) } } else { - r.RawRequest, err = http.NewRequest(r.Method, r.URL, r.bodyBuf) + // fix data race: must deep copy. + bodyBuf := bytes.NewBuffer(append([]byte{}, r.bodyBuf.Bytes()...)) + r.RawRequest, err = http.NewRequest(r.Method, r.URL, bodyBuf) } if err != nil { From e04ead057a681e80a6ff6c020d6870b19ec936a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=9F=E5=A4=A7=E9=99=86?= <10188142zhong_dalu@cn.tre-inc.com> Date: Sat, 6 May 2023 10:23:06 +0800 Subject: [PATCH 2/4] improve body copy logic --- middleware.go | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/middleware.go b/middleware.go index 712c32c5..c734ffe2 100644 --- a/middleware.go +++ b/middleware.go @@ -162,6 +162,7 @@ CL: } func createHTTPRequest(c *Client, r *Request) (err error) { + var bodyCopy *bytes.Buffer if r.bodyBuf == nil { if c.setContentLength || r.setContentLength { r.RawRequest, err = http.NewRequest(r.Method, r.URL, http.NoBody) @@ -169,9 +170,13 @@ func createHTTPRequest(c *Client, r *Request) (err error) { r.RawRequest, err = http.NewRequest(r.Method, r.URL, nil) } } else { - // fix data race: must deep copy. - bodyBuf := bytes.NewBuffer(append([]byte{}, r.bodyBuf.Bytes()...)) - r.RawRequest, err = http.NewRequest(r.Method, r.URL, bodyBuf) + // deep copy + bodyCopy = acquireBuffer() + _, err := io.Copy(bodyCopy, bytes.NewReader(r.bodyBuf.Bytes())) + if err != nil { + return err + } + r.RawRequest, err = http.NewRequest(r.Method, r.URL, bodyCopy) } if err != nil { @@ -205,9 +210,11 @@ func createHTTPRequest(c *Client, r *Request) (err error) { r.RawRequest = r.RawRequest.WithContext(r.ctx) } - bodyCopy, err := getBodyCopy(r) - if err != nil { - return err + if bodyCopy == nil { + bodyCopy, err = getRawRequestBodyCopy(r) + if err != nil { + return err + } } // assign get body func for the underlying raw request instance @@ -512,17 +519,7 @@ func saveResponseIntoFile(c *Client, res *Response) error { return nil } -func getBodyCopy(r *Request) (*bytes.Buffer, error) { - // If r.bodyBuf present, return the copy - if r.bodyBuf != nil { - bodyCopy := acquireBuffer() - if _, err := io.Copy(bodyCopy, bytes.NewReader(r.bodyBuf.Bytes())); err != nil { - // cannot use io.Copy(bodyCopy, r.bodyBuf) because io.Copy reset r.bodyBuf - return nil, err - } - return bodyCopy, nil - } - +func getRawRequestBodyCopy(r *Request) (*bytes.Buffer, error) { // Maybe body is `io.Reader`. // Note: Resty user have to watchout for large body size of `io.Reader` if r.RawRequest.Body != nil { From ca516737b14e950ff93c5cd5e6c7586ee6353486 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=9F=E5=A4=A7=E9=99=86?= <10188142zhong_dalu@cn.tre-inc.com> Date: Sat, 6 May 2023 13:07:14 +0800 Subject: [PATCH 3/4] improve --- middleware.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/middleware.go b/middleware.go index c734ffe2..fe09d091 100644 --- a/middleware.go +++ b/middleware.go @@ -162,7 +162,6 @@ CL: } func createHTTPRequest(c *Client, r *Request) (err error) { - var bodyCopy *bytes.Buffer if r.bodyBuf == nil { if c.setContentLength || r.setContentLength { r.RawRequest, err = http.NewRequest(r.Method, r.URL, http.NoBody) @@ -171,7 +170,7 @@ func createHTTPRequest(c *Client, r *Request) (err error) { } } else { // deep copy - bodyCopy = acquireBuffer() + bodyCopy := acquireBuffer() _, err := io.Copy(bodyCopy, bytes.NewReader(r.bodyBuf.Bytes())) if err != nil { return err @@ -210,19 +209,18 @@ func createHTTPRequest(c *Client, r *Request) (err error) { r.RawRequest = r.RawRequest.WithContext(r.ctx) } - if bodyCopy == nil { - bodyCopy, err = getRawRequestBodyCopy(r) + if r.bodyBuf == nil { + bodyCopy, err := getRawRequestBodyCopy(r) if err != nil { return err } - } - // assign get body func for the underlying raw request instance - r.RawRequest.GetBody = func() (io.ReadCloser, error) { if bodyCopy != nil { - return io.NopCloser(bytes.NewReader(bodyCopy.Bytes())), nil + // assign get body func for the underlying raw request instance + r.RawRequest.GetBody = func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(bodyCopy.Bytes())), nil + } } - return nil, nil } return From dabc3ee3cba2f460ad74ab99cc4879f30b43e529 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=9F=E5=A4=A7=E9=99=86?= <10188142zhong_dalu@cn.tre-inc.com> Date: Mon, 25 Sep 2023 16:13:37 +0800 Subject: [PATCH 4/4] add unit test --- middleware_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/middleware_test.go b/middleware_test.go index fef6f008..84a614b8 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -1,6 +1,7 @@ package resty import ( + "bytes" "net/url" "testing" ) @@ -227,3 +228,47 @@ func Test_parseRequestURL(t *testing.T) { }) } } + +func Test_createHTTPRequest(t *testing.T) { + type args struct { + c *Client + r *Request + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "bodyBuf is not nil, deep copy", + args: args{ + c: &Client{}, + r: func() *Request { + req := &Request{} + req.bodyBuf = bytes.NewBufferString("test") + return req + }(), + }, + wantErr: false, + }, + { + name: "bodyBuf is nil, deep copy", + args: args{ + c: &Client{}, + r: func() *Request { + req := &Request{} + req.bodyBuf = nil + return req + }(), + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := createHTTPRequest(tt.args.c, tt.args.r); (err != nil) != tt.wantErr { + t.Errorf("createHTTPRequest() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +}