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

Nil de-reference panic from attempts to access nil Retryer provider of Request object #2889

Closed
CMLivingston opened this issue Oct 14, 2019 · 4 comments · Fixed by #2934 or #2937
Closed

Comments

@CMLivingston
Copy link

Companion issue for aws/aws-dax-go#18

Version of AWS SDK for Go?

v1.25.9

Version of Go (go version)?

go1.12.9 darwin/amd64

What issue did you see?

Nil-dereference panics from attempts to access nil retryer function r.MaxRetries() of request object here: https://github.com/aws/aws-sdk-go/blob/master/aws/request/request.go#L557
This a result of passing a nil retryer to NewDaxRequest in aws-dax-go as described in aws/aws-dax-go#18.

Steps to reproduce

Use dax go client to read from a dynamo table and force a server error from Dynamo. For us, we set read capacity to 1 read unit and started getting throttled event errors from Dynamo.

A potential solution is to ensure all accesses of r.MaxRetries() only happen if aws.BoolValue(r.Retryable) is true:
https://github.com/aws/aws-sdk-go/blob/master/aws/request/request.go#L557

We also filed against the dax go project as a fix in both places would make dep management much easier for consumers of both libraries.

@diehlaws diehlaws self-assigned this Oct 16, 2019
@diehlaws diehlaws added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Oct 16, 2019
@diehlaws
Copy link
Contributor

Hi @CMLivingston, thanks for reaching out to us about this. Unfortunately I haven't been able to reproduce this using the sample code for aws/aws-dax-go modified to send GetItem requests instead of PutItem - I'm able to elicit ProvisionedThroughputExceededException errors without the client panicking on a nil dereference. Can you provide a code sample that exhibits this behavior so we can better understand how you're running into this?

@diehlaws diehlaws added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Oct 17, 2019
@VasilyFomin
Copy link

In order to reproduce it you have to do the following:

cfg := dax.
cfg.HostPorts = []string{endpoint}
cfg.Region = region
client := dax.New(cfg)
in := &dynamodb.GetItemInput{
    TableName: aws.String(table),
    Key:       key,
}
r, _ := client.GetItemRequest(in)
r.Send()

so the key point here is to use Request.Send method, which we actually don't use in our examples

@diehlaws diehlaws removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 4, 2019
jasdel added a commit to jasdel/aws-sdk-go that referenced this issue Nov 7, 2019
Adds additional default behavior to the SDK's New request constructor,
to handle the case where a nil Retryer was passed in. This error could
occur when the SDK's Request type was being used to create requests
directly, not through one of the SDK's client.

Fixes aws#2889
@jasdel
Copy link
Contributor

jasdel commented Nov 7, 2019

Thanks for letting us know about this issue. I've merged in PR #2934 which patches the ability of the SDK to more cleanly handle a nil Retryer value. This change will be included in our next tagged release.

@CMLivingston
Copy link
Author

Thanks!

aws-sdk-go-automation pushed a commit that referenced this issue Nov 8, 2019
===

### Service Client Updates
* `service/cognito-identity`: Updates service API and documentation
* `service/ecr`: Updates service documentation
  * This release contains ticket fixes for Amazon ECR.

### SDK Bugs
* `aws/request`: Ensure New request handles nil retryer ([#2934](#2934))
  * Adds additional default behavior to the SDK's New request constructor, to handle the case where a nil Retryer was passed in. This error could occur when the SDK's Request type was being used to create requests directly, not through one of the SDK's client.
  * Fixes [#2889](#2889)
aws-sdk-go-automation added a commit that referenced this issue Nov 8, 2019
Release v1.25.31 (2019-11-08)
===

### Service Client Updates
* `service/cognito-identity`: Updates service API and documentation
* `service/ecr`: Updates service documentation
  * This release contains ticket fixes for Amazon ECR.

### SDK Bugs
* `aws/request`: Ensure New request handles nil retryer ([#2934](#2934))
  * Adds additional default behavior to the SDK's New request constructor, to handle the case where a nil Retryer was passed in. This error could occur when the SDK's Request type was being used to create requests directly, not through one of the SDK's client.
  * Fixes [#2889](#2889)
@diehlaws diehlaws removed their assignment Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants