[service/s3] Close http.Response.Body in CopyObject, UploadPartCopy a…

…nd CompleteMultipartUpload operations. Fixes aws#4037
ashtuchkin · Aug 4, 2021 · 33a9063 · 33a9063
1 parent 7938281
commit 33a9063
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 10 deletions.
diff --git 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
@@ -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
@@ -11,7 +11,7 @@ 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)
 	if err != nil {
 		r.Error = awserr.NewRequestFailure(
@@ -21,6 +21,8 @@ func copyMultipartStatusOKUnmarhsalError(r *request.Request) {
 		)
 		return
 	}
+	r.HTTPResponse.Body.Close()
+
 	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
@@ -31,7 +31,8 @@ func TestCopyObjectNoError(t *testing.T) {
 <?xml version="1.0" encoding="UTF-8"?>
 <CopyObjectResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><LastModified>2009-11-23T0:00:00Z</LastModified><ETag>&quot;1da64c7f13d1e8dbeaea40b905fd586c&quot;</ETag></CopyObjectResult>`
 
-	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"),
@@ -46,10 +47,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"),
@@ -66,14 +71,18 @@ 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) {
 	const successMsg = `
 <?xml version="1.0" encoding="UTF-8"?>
 <UploadPartCopyResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><LastModified>2009-11-23T0:00:00Z</LastModified><ETag>&quot;1da64c7f13d1e8dbeaea40b905fd586c&quot;</ETag></UploadPartCopyResult>`
 
-	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"),
@@ -91,10 +100,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"),
@@ -113,14 +126,18 @@ 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) {
 	const successMsg = `
 <?xml version="1.0" encoding="UTF-8"?>
 <CompleteMultipartUploadResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Location>locationName</Location><Bucket>bucketName</Bucket><Key>keyName</Key><ETag>"etagVal"</ETag></CompleteMultipartUploadResult>`
 
-	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"),
@@ -142,10 +159,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"),
@@ -162,16 +183,26 @@ 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{
 		MaxRetries: aws.Int(0),
 		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",
@@ -182,14 +213,29 @@ 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),
 				}
 			},
 		})
 
 	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