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.

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

panic inside WithGetResponseHeader #4211

Closed
3 tasks done
minaevmike opened this issue Dec 14, 2021 · 5 comments · Fixed by #4256 or #4263
Closed
3 tasks done

panic inside WithGetResponseHeader #4211

minaevmike opened this issue Dec 14, 2021 · 5 comments · Fixed by #4256 or #4263
Assignees
Labels
bug This issue is a bug.

Comments

@minaevmike
Copy link

minaevmike commented Dec 14, 2021

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
We are using S3 API to manipulate files inside our S3 storage.
In some rare cases, WithGetResponseHeader may panic. If the request builds failed, before it was sent, request.HTTPResponse would be nil. But the complete callback would be called anyway.

Version of AWS SDK for Go?
Example: v1.41.0

Version of Go (go version)?
go 1.17.5
To Reproduce (observed behavior)
Steps to reproduce the behavior (please share code or minimal repo)

      opts1 := []func(downloader *s3manager.Downloader){
		s3manager.WithDownloaderRequestOptions(
			func(r *request.Request) {
				r.Handlers.Build.PushBackNamed(request.NamedHandler{Name: "my bad handler", Fn: func(r *request.Request) {
					r.Error = io.EOF
				}})
			}),
	}
	err := d.DownloadWithContext(ctx, buf, &r, opts1...)

Expected behavior
no panic occurs

@minaevmike minaevmike added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 14, 2021
@KaibaLopez
Copy link
Contributor

Hi @minaevmike ,
Could you provide a sample I can run and reproduce? From what I've tried I don't see the bug, but I might be doing something different.

@KaibaLopez KaibaLopez self-assigned this Dec 21, 2021
@KaibaLopez KaibaLopez added needs-reproduction This issue needs reproduction. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 21, 2021
@davidmerwin
Copy link

🔆

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Dec 22, 2021
@eitankatznelson
Copy link

We've run into this issue at our company, the issue here is that the Send method will always execute the Complete handlers, but if the request is canceled before the response object is initialized we get a nil pointer dereference causing a panic.

To reproduce, I would initiate the call in one go routine, and cancel the context in a second go routine right after a call to Send.

jasdel added a commit to jasdel/aws-sdk-go that referenced this issue Jan 25, 2022
Updates SDK Request.Send to always ensure HTTPResponse member is not
nil, and its Header and Body fields are not nil as well.

Fixes aws#4211
@jasdel
Copy link
Contributor

jasdel commented Jan 25, 2022

Thanks for reporting this issue, and providing reproduction example. I put together #4256 that fixes this bug and ensures that HTTPResponse will not be nil when Complete handlers are called returns.

@jasdel jasdel removed the needs-reproduction This issue needs reproduction. label Jan 25, 2022
jasdel added a commit that referenced this issue Jan 26, 2022
…nil (#4256)

Updates SDK Request.Send to always ensure HTTPResponse member is not
nil, and its Header and Body fields are not nil as well.

Fixes #4211
@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 Jan 27, 2022
===

### Service Client Updates
* `service/amplify`: Updates service documentation
* `service/connect`: Updates service API and documentation
* `service/ec2`: Updates service API
  * X2ezn instances are powered by Intel Cascade Lake CPUs that deliver turbo all core frequency of up to 4.5 GHz and up to 100 Gbps of networking bandwidth
* `service/kafka`: Updates service API and documentation
* `service/opensearch`: Updates service API and documentation

### SDK Bugs
* `aws/request`: Update Request Send to always ensure Request.HTTPResponse is populated.
    * Fixes [#4211](#4211)
aws-sdk-go-automation added a commit that referenced this issue Jan 27, 2022
Release v1.42.43 (2022-01-27)
===

### Service Client Updates
* `service/amplify`: Updates service documentation
* `service/connect`: Updates service API and documentation
* `service/ec2`: Updates service API
  * X2ezn instances are powered by Intel Cascade Lake CPUs that deliver turbo all core frequency of up to 4.5 GHz and up to 100 Gbps of networking bandwidth
* `service/kafka`: Updates service API and documentation
* `service/opensearch`: Updates service API and documentation

### SDK Bugs
* `aws/request`: Update Request Send to always ensure Request.HTTPResponse is populated.
    * Fixes [#4211](#4211)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
5 participants