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

Jump to bottom

S3 operations CopyObject, UploadPartCopy and CompleteMultipartUpload don't close http.Response.Body #4037

Closed
ashtuchkin opened this issue Jul 30, 2021 · 4 comments · Fixed by #4045 or #4063
Closed

S3 operations CopyObject, UploadPartCopy and CompleteMultipartUpload don't close http.Response.Body #4037

ashtuchkin opened this issue Jul 30, 2021 · 4 comments · Fixed by #4045 or #4063
Labels
bug This issue is a bug.

Comments

@ashtuchkin
Copy link
Contributor

Describe the bug
S3 operations CopyObject, UploadPartCopy and CompleteMultipartUpload use copyMultipartStatusOKUnmarhsalError unmarshal handler added in 34eb9e6 (see https://github.com/aws/aws-sdk-go/blob/v1.40.12/service/s3/customizations.go#L43).

This function reads all of r.HTTPResponse.Body into a buffer, but it's not closing it (see https://github.com/aws/aws-sdk-go/blob/v1.40.12/service/s3/statusok_error.go#L15). Not closing the response body violates http.Response contract (see docs at https://pkg.go.dev/net/http#Response), leading to inability to reuse network connection for future requests and, in our case, also breaking instrumentation for these particular operations. See https://stackoverflow.com/a/33238755/325300

Version of AWS SDK for Go?
v1.40.8

Version of Go (go version)?
1.16.5

To Reproduce (observed behavior)
I consistently reproduce this in our tests, but it'll take some time to create a minimally reproducible example. Let me know if you actually need it.

Expected behavior
HTTPResponse.Body must be closed after reading here: https://github.com/aws/aws-sdk-go/blob/v1.40.12/service/s3/statusok_error.go#L15.

Additional context
I can do a PR to fix it if you'd like.
@ashtuchkin ashtuchkin added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 30, 2021
@ashtuchkin ashtuchkin changed the title S3 operations CopyObject, UploadPartCopy and CompleteMultipartUpload don't close HTTPResponse Body. S3 operations CopyObject, UploadPartCopy and CompleteMultipartUpload don't close http.Response.Body Jul 30, 2021
@KaibaLopez
Copy link
Contributor

Hi @ashtuchkin ,
Yea, it would definitely help us, please provide one.

@KaibaLopez KaibaLopez removed the needs-triage This issue or PR still needs to be triaged. label Aug 3, 2021
@ashtuchkin
Copy link
Contributor Author

ashtuchkin commented Aug 3, 2021 via email

ashtuchkin added a commit to ashtuchkin/aws-sdk-go that referenced this issue Aug 4, 2021
@ashtuchkin
[service/s3] Close http.Response.Body in CopyObject, UploadPartCopy a…
33a9063 
…nd CompleteMultipartUpload operations. Fixes aws#4037
@ashtuchkin ashtuchkin mentioned this issue Aug 4, 2021
Merged
@ashtuchkin
Copy link
Contributor Author

Please see PR #4045

jasdel pushed a commit to ashtuchkin/aws-sdk-go that referenced this issue Aug 17, 2021
@ashtuchkin @jasdel
[service/s3] Close http.Response.Body in CopyObject, UploadPartCopy a…
a585113 
…nd CompleteMultipartUpload operations. Fixes aws#4037
jasdel pushed a commit that referenced this issue Aug 17, 2021
@ashtuchkin
Close http.Response.Body in S3 CopyObject, UploadPartCopy and Complet…
6f36a81 
…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.
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

aws-sdk-go-automation pushed a commit that referenced this issue Aug 18, 2021
Release v1.40.25 (2021-08-18)
8097ae2 
===

### Service Client Updates
* `service/codebuild`: Updates service API and documentation
  * CodeBuild now allows you to make the build results for your build projects available to the public without requiring access to an AWS account.
* `service/route53`: Updates service documentation
  * Documentation updates for route53
* `service/route53resolver`: Updates service documentation
* `service/runtime.sagemaker`: Updates service API and documentation
* `service/sagemaker`: Updates service API and documentation
  * Amazon SageMaker now supports Asynchronous Inference endpoints. Adds PlatformIdentifier field that allows Notebook Instance creation with different platform selections. Increases the maximum number of containers in multi-container endpoints to 15. Adds more instance types to InstanceType field.

### SDK Bugs
* `service/s3`: Close http.Response.Body in CopyObject, UploadPartCopy and CompleteMultipartUpload operations
  * Fixes [#4037](#4037)
Merged
aws-sdk-go-automation added a commit that referenced this issue Aug 18, 2021
@aws-sdk-go-automation
Release v1.40.25 (2021-08-18) (#4063)
5806617 
Release v1.40.25 (2021-08-18)
===

### Service Client Updates
* `service/codebuild`: Updates service API and documentation
  * CodeBuild now allows you to make the build results for your build projects available to the public without requiring access to an AWS account.
* `service/route53`: Updates service documentation
  * Documentation updates for route53
* `service/route53resolver`: Updates service documentation
* `service/runtime.sagemaker`: Updates service API and documentation
* `service/sagemaker`: Updates service API and documentation
  * Amazon SageMaker now supports Asynchronous Inference endpoints. Adds PlatformIdentifier field that allows Notebook Instance creation with different platform selections. Increases the maximum number of containers in multi-container endpoints to 15. Adds more instance types to InstanceType field.

### SDK Bugs
* `service/s3`: Close http.Response.Body in CopyObject, UploadPartCopy and CompleteMultipartUpload operations
  * Fixes [#4037](#4037)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug This issue is a bug.
Projects
None yet
Milestone
No milestone
2 participants
@ashtuchkin @KaibaLopez