Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch reading from S3 to io.Copy from io.ReadFull #4225

Merged
merged 2 commits into from
Mar 30, 2018
Merged

Switch reading from S3 to io.Copy from io.ReadFull #4225

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
332657c
Switch reading from S3 to io.Copy from io.ReadFull
jefferai Mar 30, 2018
e90c7cf
Add some extra efficiency
jefferai Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions physical/s3/s3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,16 +188,20 @@ func (s *S3Backend) Get(ctx context.Context, key string) (*physical.Entry, error
if resp == nil {
return nil, fmt.Errorf("got nil response from S3 but no error")
}
defer resp.Body.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be more defensive to do this right before line 178? Something like, if resp != nil { defer resp.Body.Close() }? Then there'd be absolutely no way there could be a memory leak if we hit 404's a lot, and if for some reason the AWS client still returned a body. I know for the http.Client that doesn't happen, but I don't know if the AWS client maintains the same contract. I can't find anything saying either way in their docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, considering we're thinking of returning data with 404s. I'll update.


data := make([]byte, *resp.ContentLength)
_, err = io.ReadFull(resp.Body, data)
data := bytes.NewBuffer(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To retain the more efficient pre-allocation when ContentLength is known (presumably the normal case), consider:

	var cap int64
	if resp.ContentLength != nil {
		cap = *resp.ContentLength
	}

	data := bytes.NewBuffer(make([]byte, 0, cap))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at that as a "efficiency doesn't really matter because network aspects will dominate the total time" but I guess if it's a 20mb object it actually could, so I'll do that.

if resp.ContentLength != nil {
data = bytes.NewBuffer(make([]byte, 0, *resp.ContentLength))
}
_, err = io.Copy(data, resp.Body)
if err != nil {
return nil, err
}

ent := &physical.Entry{
Key: key,
Value: data,
Value: data.Bytes(),
}

return ent, nil
Expand Down