From e7830bfd3e466826671e7b864a98647e17998ab6 Mon Sep 17 00:00:00 2001 From: Gareth Watts Date: Fri, 29 May 2020 16:26:42 +0000 Subject: [PATCH 1/4] service/s3/s3crypto Fix request retries Closing and removing the temp file after send causes retry requests to fail with an error such as: filestore: SerializationError: failed to prepare body for retry caused by: SerializationError: failed to reset request body caused by: SerializationError: failed to get next request body reader caused by: seek /tmp/213721967: file already closed I believe the Complete handler is guaranteed to be called finally after retries attempts have succeeded or been exhausted. --- service/s3/s3crypto/helper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/s3/s3crypto/helper.go b/service/s3/s3crypto/helper.go index be40fc90319..949286923a7 100644 --- a/service/s3/s3crypto/helper.go +++ b/service/s3/s3crypto/helper.go @@ -19,7 +19,7 @@ func getWriterStore(req *request.Request, path string, useTempFile bool) (io.Rea return nil, err } - req.Handlers.Send.PushBack(func(r *request.Request) { + req.Handlers.Complete.PushBack(func(r *request.Request) { // Close the temp file and cleanup f.Close() os.Remove(f.Name()) From 575553281c45298a5c4afe0cd1345e4eb382f170 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Mon, 20 Jul 2020 09:26:07 -0700 Subject: [PATCH 2/4] add unit test for cleanup --- service/s3/s3crypto/helper_test.go | 38 ++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/service/s3/s3crypto/helper_test.go b/service/s3/s3crypto/helper_test.go index 26993bcdc97..03f22bb8ee9 100644 --- a/service/s3/s3crypto/helper_test.go +++ b/service/s3/s3crypto/helper_test.go @@ -2,6 +2,7 @@ package s3crypto import ( "bytes" + "io/ioutil" "net/http" "os" "testing" @@ -118,6 +119,43 @@ func TestGetWriterStore_TempFile(t *testing.T) { } } +func TestGetWriterStore_TempFileWithRetry(t *testing.T) { + responses := []*http.Response{ + &http.Response{StatusCode: 500, Header: http.Header{}, Body: ioutil.NopCloser(&bytes.Buffer{})}, + &http.Response{StatusCode: 200, Header: http.Header{}, Body: ioutil.NopCloser(&bytes.Buffer{})}, + } + s := awstesting.NewClient(aws.NewConfig().WithMaxRetries(10)) + s.Handlers.Validate.Clear() + s.Handlers.Send.Clear() // mock sending + s.Handlers.Send.PushBack(func(r *request.Request) { + r.HTTPResponse = responses[0] + responses = responses[1:] + }) + type testData struct { + Data string + } + out := &testData{} + r := s.NewRequest(&request.Operation{Name: "Operation"}, nil, out) + f, err := getWriterStore(r, "", true) + if err != nil { + t.Fatalf("expected no error, but received %v", err) + } + tempFile, ok := f.(*os.File) + if !ok { + t.Fatal("io.ReadWriteSeeker expected to be *os.file") + } + err = r.Send() + if err != nil { + t.Errorf("expected no error, but received %v", err) + } + if _, err := os.Stat(tempFile.Name()); !os.IsNotExist(err) { + t.Errorf("expected temp file be deleted, but still exists %v", tempFile.Name()) + } + if v := len(responses); v != 0 { + t.Errorf("expect all retries to be used, have %v remaining", v) + } +} + func TestGetWriterStore_Memory(t *testing.T) { response := http.Response{StatusCode: 200} s := awstesting.NewClient(aws.NewConfig().WithMaxRetries(10)) From ec6ee22d4f4e36c665e69bd81d176b3ef380e1e1 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Mon, 20 Jul 2020 09:26:18 -0700 Subject: [PATCH 3/4] add change log --- CHANGELOG_PENDING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 8a1927a39ca..94057b39473 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -3,3 +3,5 @@ ### SDK Enhancements ### SDK Bugs +* `service/s3/s3crypto`: Fix client's temporary file buffer error on retry ([#3344](https://github.com/aws/aws-sdk-go/pull/3344)) + * Fixes the Crypto client's temporary file buffer cleanup returning an error when the request is retried. From a9ca638ce6efb7fd93dbbeb2d8d600f2d9b9f117 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Mon, 20 Jul 2020 13:42:11 -0700 Subject: [PATCH 4/4] fixup gofmt --- service/s3/s3crypto/helper_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/service/s3/s3crypto/helper_test.go b/service/s3/s3crypto/helper_test.go index 03f22bb8ee9..d9f07ca70d6 100644 --- a/service/s3/s3crypto/helper_test.go +++ b/service/s3/s3crypto/helper_test.go @@ -121,8 +121,8 @@ func TestGetWriterStore_TempFile(t *testing.T) { func TestGetWriterStore_TempFileWithRetry(t *testing.T) { responses := []*http.Response{ - &http.Response{StatusCode: 500, Header: http.Header{}, Body: ioutil.NopCloser(&bytes.Buffer{})}, - &http.Response{StatusCode: 200, Header: http.Header{}, Body: ioutil.NopCloser(&bytes.Buffer{})}, + {StatusCode: 500, Header: http.Header{}, Body: ioutil.NopCloser(&bytes.Buffer{})}, + {StatusCode: 200, Header: http.Header{}, Body: ioutil.NopCloser(&bytes.Buffer{})}, } s := awstesting.NewClient(aws.NewConfig().WithMaxRetries(10)) s.Handlers.Validate.Clear()