search

aws / aws-dax-go

AWS DAX SDK for the Go programming language. https://aws.amazon.com/dynamodb/dax
Apache License 2.0
47 stars 48 forks source link

Nil de-reference panic from nil Retryer object passed during dax request construction #18

Closed CMLivingston closed 4 years ago

CMLivingston commented 4 years ago

We are seeing a nil dereference panic due to a bug in the dax internal client (version v1.1.2) that causes a process to panic when receiving any HTTP errors from usage of the client. In this case, aws-sdk-go attempts to log debug information when errors are present on the request object and it attempts to de-reference a nil field on the Request object constructed in dax client code here: https://github.com/aws/aws-dax-go/blob/master/dax/internal/client/cluster.go#L255

The nil parameter is the retryer assigned to the embedded Retryer field of Request, which is what provides the method that is being called when the nil dereference panic happens in aws-sdk-go on line https://github.com/aws/aws-sdk-go/blob/master/aws/request/request.go#L557.

Seems like there are two potential solutions: 1) In aws-dax-go, change this line to pass in awsapi.NoAWSRetry{} instead of nil if consumers do not provide a retryer object of their own (and just use that one if they do): https://github.com/aws/aws-dax-go/blob/master/dax/internal/client/cluster.go#L255 2) In aws-sdk-go, ensure that all accesses of r.MaxRetries() only happen if aws.BoolValue(r.Retryable) is true like they do elsewhere. https://github.com/aws/aws-sdk-go/blob/master/aws/request/request.go#L557

A fix in both places would be great so we can upgrade each library independently and flexibly .

VasilyFomin commented 4 years ago

Thanks for reporting this.

  1. Looks like v1.23.22 added NoOpRetryer and we should see if we can upgrade aws-dax-go
  2. We'll see what we can do to get it fixed in the aws-sdk-go

Is this a blocking issue for you at the moment?

CMLivingston commented 4 years ago

No problem! I also filed https://github.com/aws/aws-sdk-go/issues/2889. This is not blocking right now - we implemented a workaround by PushFronting a request handler that intercepts potentially problematic requests and fills the retryer provider. Along the lines of:


...PushFront(func(r *request.Request) {
        if r.Retryer == nil {
            r.Retryer = ourCustomClient.Retryer
        }
    })
CMLivingston commented 4 years ago

We are still getting panics from the nil Retryer provider. After further investigation, found that we cannot work around this without forking the project to use the NoOpRetryer due to the private internal/client implementations behind dax.New. An upgrade would be greatly appreciated.

VasilyFomin commented 4 years ago

Thanks for the priority update. We're looking into prioritizing this.

VasilyFomin commented 4 years ago

Actually, now that I thought about it, I think we should make the fix on the aws-sdk side, as opposed to dax, because fixing it on the dax end will be binary incompatible change and will force our customers to upgrade to the latest aws-sdk version.

I'm going to follow up with the sdk team.

VasilyFomin commented 4 years ago

This was fixed on the aws-sdk-go side and I'm resolving this issue. We're going to also include NoOpRetryer with the next major release.