RequestTimeout is not respected when API is called with context

[adriantam]

We have set the RequestTimeout to 75ms in our DAX client. However, when TCP connection is re-establshed, we still see some DAX request in the 6s range (which based on our discussion with AWS team are caused by connection being re-established).

Looking at the Go SDK, it appears that the issue is due to SDK not respecting the request timeout if it is called with context.

See https://github.com/aws/aws-dax-go/blob/264dfde337bc592b0ada5a8c587f14f610cd922a/dax/service.go#L163

Please note that we use QueryWithContext as per https://pkg.go.dev/github.com/zabawaba99/aws-dax-go/dax#Dax.QueryWithContext.

[anudeeb]

Are you setting timeouts in the context that's passed to DAX SDK? If you are explicitly passing in a context, it should look like below if you want to configure 50 ms timeout:

ctxTimeout, cancel := context.WithTimeout(context.Background(), time.Millisecond*50)  

client.GetItemWithContext(ctxTimeout, in)

When context is passed to the SDK, we will honour the timeout that's set in the context and will not consider the RequestTimeout field value which was set during client initialisation.

That's why in the below code, the request timeout is overridden if context is provided - https://github.com/aws/aws-dax-go/blob/264dfde337bc592b0ada5a8c587f14f610cd922a/dax/service.go#L163

I executed couple of tests to validate this behaviour and it works as expected:

1. I set the RequestTimeout configuration as 10 ms and while calling GetItemWithContext I provided context.Background() (infinite timeout)

   • There were no timeouts and we would see high latencies (note: simulated high server-side latencies by manually adding sleep on server-side)
   • 

2. I set the RequestTimeout configuration as 5 seconds and while calling GetItemWithContext, passed in context with 50 ms as timeout - context.WithTimeout(context.Background(), time.Millisecond*50)

   • I saw that client-side latencies were capped at 50 ms
   • 

[adriantam]

Are you setting timeouts in the context that's passed to DAX SDK? If you are explicitly passing in a context, it should look like below if you want to configure 50 ms timeout:

ctxTimeout, cancel := context.WithTimeout(context.Background(), time.Millisecond*50)  

client.GetItemWithContext(ctxTimeout, in)

When context is passed to the SDK, we will honour the timeout that's set in the context and will not consider the RequestTimeout field value which was set during client initialisation.

That's why in the below code, the request timeout is overridden if context is provided -

https://github.com/aws/aws-dax-go/blob/264dfde337bc592b0ada5a8c587f14f610cd922a/dax/service.go#L163

I executed couple of tests to validate this behaviour and it works as expected:

1. I set the RequestTimeout configuration as 10 ms and while calling GetItemWithContext I provided context.Background() (infinite timeout)

   • There were no timeouts and we would see high latencies (note: simulated high server-side latencies by manually adding sleep on server-side)
   • 

2. I set the RequestTimeout configuration as 5 seconds and while calling GetItemWithContext, passed in context with 50 ms as timeout - context.WithTimeout(context.Background(), time.Millisecond*50)

   • I saw that client-side latencies were capped at 50 ms
   • 

We did not set the timeout in the context that we passed into the QueryWithContext request.

When we call the API, we do something like

	cfg := dax.DefaultConfig()
        cfg.RequestTimeout = 75 * time.Millisecond
        client, err := dax.New(cfg)
        ...
        ctx := context.Background()
        // some code to add in our own context info
        output, err := client.QueryWithContext(ctx, input, opts)

There was nothing in the documentation to suggest that Config's RequestTimeout (i.e., https://github.com/aws/aws-dax-go/blob/264dfde337bc592b0ada5a8c587f14f610cd922a/dax/service.go#L49) will only be used if there is no context provided.

[jon-whit]

Why have the RequestTimeout config if the context dominates, especially if context.Background() overrides the top-level config setting?

Perhaps I may suggest that RequestTimeout is just dropped altogether in favor of the more idiomatic Go style of providing solely the context with timeout.

[anudeeb]

Thank you for the feedback! We'll update the documentation in the next release to call out that the top-level configs are not honoured if the context is set.

Perhaps I may suggest that RequestTimeout is just dropped altogether in favor of the more idiomatic Go style of providing solely the context with timeout.

That's a good suggestion but unfortunately if we make that change now, the change will be backwards incompatible.

[jon-whit]

That's a good suggestion but unfortunately if we make that change now, the change will be backwards incompatible.

@anudeeb looking forward to github.com/aws/aws-dax-go/v2 😄 !