Switch reading from S3 to io.Copy from io.ReadFull #4225
Conversation
If the Content-Length header wasn't being sent back, the current behavior could panic. It's unclear when it will not be sent; it appears to be CORS dependent. But this works around it by not trying to preallocate a buffer of a specific size and instead just read until EOF. In addition I noticed that Close wasn't being called. https://docs.aws.amazon.com/sdk-for-go/api/service/s3/#GetObjectOutput specifies that Body is an io.ReadCloser so I added a call to Close. Fixes #4222
physical/s3/s3.go
Outdated
| data := make([]byte, *resp.ContentLength) | ||
| _, err = io.ReadFull(resp.Body, data) | ||
| data := bytes.NewBuffer(nil) | ||
| _, err = io.Copy(data, bytes.NewBuffer(resp.Body)) |
There was a problem hiding this comment.
I believe this should just be resp.Body w/o the bytes.NewBuffer()
There was a problem hiding this comment.
Oh yeah good point, I did that before looking up whether it was a byte slice or reader and forgot to go back and change it.
|
|
||
| data := make([]byte, *resp.ContentLength) | ||
| _, err = io.ReadFull(resp.Body, data) | ||
| data := bytes.NewBuffer(nil) |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
|
@kalafut Fixed, can you ✔️ ? |
| @@ -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() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's fair, considering we're thinking of returning data with 404s. I'll update.
If the Content-Length header wasn't being sent back, the current
behavior could panic. It's unclear when it will not be sent; it appears
to be CORS dependent. But this works around it by not trying to
preallocate a buffer of a specific size and instead just read until EOF.
In addition I noticed that Close wasn't being called.
https://docs.aws.amazon.com/sdk-for-go/api/service/s3/#GetObjectOutput
specifies that Body is an io.ReadCloser so I added a call to Close.
Fixes #4222