From 6f36a81b9237f3161ba67748e23cde78c32802b8 Mon Sep 17 00:00:00 2001 From: Alexander Shtuchkin Date: Tue, 17 Aug 2021 16:35:37 -0400 Subject: [PATCH] Close http.Response.Body in S3 CopyObject, UploadPartCopy and CompleteMultipartUpload operations. (#4045) Fixes #4037. See that issue for more details. Close error is ignored as it has no side effects and there's nothing we can do about it. --- CHANGELOG_PENDING.md | 2 + service/s3/customizations.go | 2 +- service/s3/statusok_error.go | 7 +++- service/s3/statusok_error_test.go | 61 +++++++++++++++++++++++++++---- 4 files changed, 62 insertions(+), 10 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 8a1927a39ca..8167806730d 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -3,3 +3,5 @@ ### SDK Enhancements ### SDK Bugs +* `service/s3`: Close http.Response.Body in CopyObject, UploadPartCopy and CompleteMultipartUpload operations + * Fixes [#4037](https://github.com/aws/aws-sdk-go/issues/4037) diff --git a/service/s3/customizations.go b/service/s3/customizations.go index ce87ab32017..bce45a3ea8d 100644 --- a/service/s3/customizations.go +++ b/service/s3/customizations.go @@ -40,7 +40,7 @@ func defaultInitRequestFn(r *request.Request) { // Auto-populate LocationConstraint with current region r.Handlers.Validate.PushFront(populateLocationConstraint) case opCopyObject, opUploadPartCopy, opCompleteMultipartUpload: - r.Handlers.Unmarshal.PushFront(copyMultipartStatusOKUnmarhsalError) + r.Handlers.Unmarshal.PushFront(copyMultipartStatusOKUnmarshalError) r.Handlers.Unmarshal.PushBackNamed(s3err.RequestFailureWrapperHandler()) case opPutObject, opUploadPart: r.Handlers.Build.PushBack(computeBodyHashes) diff --git a/service/s3/statusok_error.go b/service/s3/statusok_error.go index 247770e4c88..096adc091dd 100644 --- a/service/s3/statusok_error.go +++ b/service/s3/statusok_error.go @@ -11,16 +11,21 @@ import ( "github.com/aws/aws-sdk-go/internal/sdkio" ) -func copyMultipartStatusOKUnmarhsalError(r *request.Request) { +func copyMultipartStatusOKUnmarshalError(r *request.Request) { b, err := ioutil.ReadAll(r.HTTPResponse.Body) + r.HTTPResponse.Body.Close() if err != nil { r.Error = awserr.NewRequestFailure( awserr.New(request.ErrCodeSerialization, "unable to read response body", err), r.HTTPResponse.StatusCode, r.RequestID, ) + // Note, some middleware later in the stack like restxml.Unmarshal expect a valid, non-closed Body + // even in case of an error, so we replace it with an empty Reader. + r.HTTPResponse.Body = ioutil.NopCloser(bytes.NewBuffer(nil)) return } + body := bytes.NewReader(b) r.HTTPResponse.Body = ioutil.NopCloser(body) defer body.Seek(0, sdkio.SeekStart) diff --git a/service/s3/statusok_error_test.go b/service/s3/statusok_error_test.go index dc8176aa7ce..dc9f988d36c 100644 --- a/service/s3/statusok_error_test.go +++ b/service/s3/statusok_error_test.go @@ -32,7 +32,8 @@ func TestCopyObjectNoError(t *testing.T) { 2009-11-23T0:00:00Z"1da64c7f13d1e8dbeaea40b905fd586c"` - res, err := newCopyTestSvc(successMsg).CopyObject(&s3.CopyObjectInput{ + var responseBodyClosed bool + res, err := newCopyTestSvc(successMsg, &responseBodyClosed).CopyObject(&s3.CopyObjectInput{ Bucket: aws.String("bucketname"), CopySource: aws.String("bucketname/exists.txt"), Key: aws.String("destination.txt"), @@ -47,10 +48,14 @@ func TestCopyObjectNoError(t *testing.T) { if e, a := lastModifiedTime, *res.CopyObjectResult.LastModified; !e.Equal(a) { t.Errorf("expected %v, but received %v", e, a) } + if !responseBodyClosed { + t.Error("http response body is not closed") + } } func TestCopyObjectError(t *testing.T) { - _, err := newCopyTestSvc(errMsg).CopyObject(&s3.CopyObjectInput{ + var responseBodyClosed bool + _, err := newCopyTestSvc(errMsg, &responseBodyClosed).CopyObject(&s3.CopyObjectInput{ Bucket: aws.String("bucketname"), CopySource: aws.String("bucketname/doesnotexist.txt"), Key: aws.String("destination.txt"), @@ -67,6 +72,9 @@ func TestCopyObjectError(t *testing.T) { if e, a := "message body", e.Message(); e != a { t.Errorf("expected %s, but received %s", e, a) } + if !responseBodyClosed { + t.Error("http response body is not closed") + } } func TestUploadPartCopySuccess(t *testing.T) { @@ -74,7 +82,8 @@ func TestUploadPartCopySuccess(t *testing.T) { 2009-11-23T0:00:00Z"1da64c7f13d1e8dbeaea40b905fd586c"` - res, err := newCopyTestSvc(successMsg).UploadPartCopy(&s3.UploadPartCopyInput{ + var responseBodyClosed bool + res, err := newCopyTestSvc(successMsg, &responseBodyClosed).UploadPartCopy(&s3.UploadPartCopyInput{ Bucket: aws.String("bucketname"), CopySource: aws.String("bucketname/doesnotexist.txt"), Key: aws.String("destination.txt"), @@ -92,10 +101,14 @@ func TestUploadPartCopySuccess(t *testing.T) { if e, a := lastModifiedTime, *res.CopyPartResult.LastModified; !e.Equal(a) { t.Errorf("expected %v, but received %v", e, a) } + if !responseBodyClosed { + t.Error("http response body is not closed") + } } func TestUploadPartCopyError(t *testing.T) { - _, err := newCopyTestSvc(errMsg).UploadPartCopy(&s3.UploadPartCopyInput{ + var responseBodyClosed bool + _, err := newCopyTestSvc(errMsg, &responseBodyClosed).UploadPartCopy(&s3.UploadPartCopyInput{ Bucket: aws.String("bucketname"), CopySource: aws.String("bucketname/doesnotexist.txt"), Key: aws.String("destination.txt"), @@ -114,6 +127,9 @@ func TestUploadPartCopyError(t *testing.T) { if e, a := "message body", e.Message(); e != a { t.Errorf("expected %s, but received %s", e, a) } + if !responseBodyClosed { + t.Error("http response body is not closed") + } } func TestCompleteMultipartUploadSuccess(t *testing.T) { @@ -121,7 +137,8 @@ func TestCompleteMultipartUploadSuccess(t *testing.T) { locationNamebucketNamekeyName"etagVal"` - res, err := newCopyTestSvc(successMsg).CompleteMultipartUpload(&s3.CompleteMultipartUploadInput{ + var responseBodyClosed bool + res, err := newCopyTestSvc(successMsg, &responseBodyClosed).CompleteMultipartUpload(&s3.CompleteMultipartUploadInput{ Bucket: aws.String("bucketname"), Key: aws.String("key"), UploadId: aws.String("uploadID"), @@ -143,10 +160,14 @@ func TestCompleteMultipartUploadSuccess(t *testing.T) { if e, a := "locationName", *res.Location; e != a { t.Errorf("expected %s, but received %s", e, a) } + if !responseBodyClosed { + t.Error("http response body is not closed") + } } func TestCompleteMultipartUploadError(t *testing.T) { - _, err := newCopyTestSvc(errMsg).CompleteMultipartUpload(&s3.CompleteMultipartUploadInput{ + var responseBodyClosed bool + _, err := newCopyTestSvc(errMsg, &responseBodyClosed).CompleteMultipartUpload(&s3.CompleteMultipartUploadInput{ Bucket: aws.String("bucketname"), Key: aws.String("key"), UploadId: aws.String("uploadID"), @@ -163,9 +184,12 @@ func TestCompleteMultipartUploadError(t *testing.T) { if e, a := "message body", e.Message(); e != a { t.Errorf("expected %s, but received %s", e, a) } + if !responseBodyClosed { + t.Error("http response body is not closed") + } } -func newCopyTestSvc(errMsg string) *s3.S3 { +func newCopyTestSvc(errMsg string, responseBodyClosed *bool) *s3.S3 { const statusCode = http.StatusOK svc := s3.New(unit.Session, &aws.Config{ @@ -173,6 +197,13 @@ func newCopyTestSvc(errMsg string) *s3.S3 { SleepDelay: func(time.Duration) {}, }) + closeChecker := func() error { + if responseBodyClosed != nil { + *responseBodyClosed = true + } + return nil + } + svc.Handlers.Send.Swap(corehandlers.SendHandler.Name, request.NamedHandler{ Name: "newCopyTestSvc", @@ -183,7 +214,7 @@ func newCopyTestSvc(errMsg string) *s3.S3 { Status: http.StatusText(statusCode), StatusCode: statusCode, Header: http.Header{}, - Body: ioutil.NopCloser(strings.NewReader(errMsg)), + Body: newFuncCloser(strings.NewReader(errMsg), closeChecker), } }, }) @@ -191,6 +222,20 @@ func newCopyTestSvc(errMsg string) *s3.S3 { return svc } +// funcCloser converts io.Reader into io.ReadCloser by adding a custom function that is called on Close() +type funcCloser struct { + io.Reader + closeFn func() error +} + +func newFuncCloser(reader io.Reader, closeFn func() error) *funcCloser { + return &funcCloser{Reader: reader, closeFn: closeFn} +} + +func (f funcCloser) Close() error { + return f.closeFn() +} + func TestStatusOKPayloadHandling(t *testing.T) { cases := map[string]struct { Header http.Header