Close http.Response.Body in S3 CopyObject, UploadPartCopy and Complet…

…eMultipartUpload 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

@@ -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)

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)

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)

service/s3/statusok_error_test.go

@@ -32,7 +32,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"),
@@ -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,14 +72,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"),
@@ -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,14 +127,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"),
@@ -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,16 +184,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",
@@ -183,14 +214,28 @@ 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