Add context support to clients

[emsearcy]

Context is used by http.Request to interrupt in-flight requests when needed. It is also needed by various APM tools that use the context to store the current application state for instrumenting HTTP call performance.

This adds support for context as unobtrusively as possible, by adding a Client.WithContext(ctx) call. This behaves similarly to the http library, except that we apply WithContext to the Client.

An example use case in a web service would be to create the client, as per the README, in the main() function, and have this client be accessible to http.Handlers (passed, or a global). Then, the http.Handler can call client := globalClient.WithContext(req.Context()) (where req is the http.Request passed to the handler). This allows http calls made to DNSimple's API to use the current context, while also allowing a concurrency-safe global client that allows for keepalive and SSL renegotiation across multiple invocations of the web service.

[weppos]

Thanks for the patch @emsearcy . I actually have a plan (I was quite sure it was already done, but maybe I had the branch locally and never pushed it) to add context to each method by changing

func (s *DomainsService) ListDomains(accountID string, options *DomainListOptions) (*domainsResponse, error) {

into

func (s *DomainsService) ListDomains(ctx context.Context, accountID string, options *DomainListOptions) (*domainsResponse, error) {

This would apply the context to the specific call rather than to the entire client.

Would this approach work for you?

[emsearcy]

Yeah that would work. I know “don’t break backwards compatibility” is a thing with Go, though, which is why you see a lot of people creating function signatures like MakeSomeRequestWithContext(ctx, ...) so they add new methods without breaking compatibility for the old methods ... but then you’re going to double the number of methods ;-)

Regardless, any of these would work for me.

[bithavoc]

Agree @emsearcy, it's actually what AWS SDK does to offer legacy and modern versions of the same operation with contexts without requiring breaking compatibility or major version bumps:

• RunInstances
• RunInstancesWithContext

This also helps with request cancellations.

Anyone owning this @weppos ?

[weppos]

@emsearcy @bithavoc I apologize for the silence here, we had some other work that kept us quite busy. We'll do out best to restart a periodic maintenance of the API clients, which will include reviewing open PRs more consistently.

I need a bit more time to think about the implications, as well add corresponding tests should we decide to go ahead with this approach. The API client is not yet 1.x hence I would be ok to change the signature.

I have some mixed feelings. On one side I appreciate the simplicity of this solution. I'm not totally convinced about assigning a context to a client, given Go makes it available to the request to be able to reuse the client in different contexts. At the time being I'm leaning towards going ahead with this change, so that we introduce context capabilities, and defer a larger change of the client for later.

I'll keep you posted.

[emsearcy]

I appreciate you considering this!

given Go makes it available to the request to be able to reuse the client in different contexts.

That's why you should implement it using my PR, not by changing it so that the context is provided when the client is created. The client should be global.

Here's an example of what we've had in production for about 8 months now. Basically, if you want to use something like AWS X-Ray or NewRelic to provide APM on your HTTP calls, you have a special client that wraps RoundTripper, so we pass that to dnsimple.NewClient(). But to actually make requests with that client, it must also must have the context of the current request, because the context is where those SDKs store their per-request state.

If you implement context setting on dnsimple.NewClient, then if you define the client globally you'd have to use context.Background() which is long-lived, but does not add any benefits of context (request cancelations, tracking state for APM). So, to use actual contexts with NewClient means people would have to NewClient per request (once the context for that request is available), but doing that means loosing all the performance benefits of reusing the same client across multiple requests (http keepalive and SSL session resumption!).

X-Ray example with http keepalive and SSL session resumption to DNSimple API across requests:

package main

import (
       "context"
       "net/http"

        "github.com/aws/aws-xray-sdk-go/xray"
        // use fork with PR 82 applied
        "github.com/emsearcy/dnsimple-go/dnsimple"
        "golang.org/x/oauth2"
)

var dnsimpleClient *dnsimple.Client

func main() {
        // Create the DynamoDB client with X-Ray. Cannot be called directly without
        // calling dnsimpleClient.WithContext, because X-Ray will panic if it cannot
        // find the parent segment.
        ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "****"})
        // Background context would be used for token refreshes, which don't happen with StaticTokenSource
        tc := oauth2.NewClient(context.Background(), ts)

        xc := xray.Client(tc)
        dnsimpleClient = dnsimple.NewClient(xc)

        // hook up response handler / start listening etc
        // [...]
}

func someRequestHandler(r *http.Request) {
        // in non-http-server use cases, ctx may just be passed to function directly
        ctx := r.Context()

        // use client with X-Ray instrumentation of requests
        client := dnsimpleClient.WithContext(ctx)
        // client.Domains.CreateDomain(...)
}

[weppos]

@emsearcy @bithavoc I started to do some work in a parallel branch #90. I hope I can merge the changes in a not too distant future.

@emsearcy are you still using your fork?

[weppos]

Closed by #90