Support for context timeout/deadline for requests in underlying HTTP client

[alexsniffin]

Is your feature request related to a problem? Please describe. The default behavior for whenever a request is made is using http.NewRequest (from here) which will use the default HTTP clients timeout (from here). This will block the caller and force them to wait until the request either fulfills or times out.

Describe the solution you'd like Support passing in a context throughout different request types, e.g.:

ctx, cancelFunc := context.WithDeadline(context.Background(), someTime)
client.GetDocumentAsStream(ctx, s.NewGetDocumentOptions(someDatabaseName, someID))

By supporting this, the request can be canceled so that the caller isn't blocked by the library if they have their own timeout they need to support.

[ricellis]

I'm going to transfer this to https://github.com/IBM/go-sdk-core so it can be taken under consideration there.

[padamstx]

I think we would want to avoid a change that would modify the signature of generated operation methods (e.g. GetDocumentAsStream() in the example above). So, instead of adding the new Context argument to each operation method, perhaps we could do one of the following:

1. Add a new field to each operation's "options" model struct so that the Context argument/option is handled just like the "Headers" field that allows the caller to set custom headers for a particular operation invocation.
2. Allow the client context to be set on the service instance and then used on each subsequent operation invoked using that service instance.

1 would make sense if a Context is more closely related to a particular request.

2 might make sense if a particular Context would typically be re-used for multiple operation invocations using a particular service instance.

I think I'm leaning toward #1. Comments?

[ricellis]

I think we would want to avoid a change that would modify the signature of generated operation methods

I agree with this sentiment. However, the Go docs for Context suggest that it shouldn't be stored in a struct:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx

So I think perhaps the Go way to do this would be to either generate the additional functions with the Context parameter or to split the request/response such that the Request.WithContext was accessible to the caller.

However, I do think the option 1 approach of having the Context more closely related to a particular request is the right one rather than having it associated with the service instance.

[padamstx]

@ricellis I saw that warning against structs but the paragraph before that seemed to indicate the warning was simply so that static analysis tools could better "see" the use of the Context within function signatures. Here's the larger quote:

Programs that use Contexts should follow these rules to keep interfaces consistent across packages and enable static analysis tools to check context propagation:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx

So, I'm not sure how much stock to put in that warning given our current Go SDK code generation model. It would be far less disruptive to simply add a new RequestContext field (perhaps with a setter) to each generated options model struct (similar to the Headers field), then use that field within the generated operation while constructing the http.Request object.

The alternative to that would be that we'd need to continue generating the operation methods as we are now (for compatibility's sake), plus a second flavor of the operation that includes the Context parameter:

func (svc *MyService) CreateFoo(createFooOptions *CreateFooOptions) (result *Foo, response *DetailedResponse, err error) {
        return svc.CreateFooWithContext(context.TODO, createFooOptions)
}

func (svc *MyService) CreateFooWithContext(ctx *context.Context, createFooOptions *CreateFooOptions) (result *Foo, response *DetailedResponse, err error) {
        ...
}

cc: @mkistler

[ricellis]

I understand, I read it as two separate reasons:

1. to keep interfaces consistent across packages
2. to enable static analysis tools to check context propagation

I agree that neither of those are as stark as something suggesting it a Context would not work if used in a struct, but I do think that both are important reasons from a "help the user" perspective.

Of course I totally agree regarding the level of disruption and I guess that needs to be traded off against user feel/natural Go style.

[padamstx]

Ok, so we have two different options:

1. Add a new RequestContext field to each generated options model struct and otherwise maintain the current operation method signatures.
2. Introduce a new <operation>WithContext flavor of each operation method that adds a Context parameter as the first param to the function, as in the example above.

Unless there is a strong objection to option 1, I think I'd prefer that option simply due to the fact that it would mean less disruption to our existing approach in Go generation.

[padamstx]

Status: PR (https://github.com/IBM/go-sdk-core/pull/79) has been opened for the required changes in the Go core.
This adds support for setting a Context instance on the RequestBuilder struct so that it will be propagated to the http.Request instance that is eventually created by the RequestBuilder.Build() method.

[padamstx]

@ricellis @alexsniffin Uh oh... my code change in the Go core to use the http.NewRequestWithContext() method failed to build due to the fact that the go core build currently uses 1.12 as the minimum version of Go, whereas the NewRequestWithContext() method was added in Go v 1.13.

This means that I'll need to do the following in order to complete this feature:

1. Declare a new major version (v5) of the Go core which supports Go 1.13+, and deprecate the v4 major version.
2. The new version of the SDK generator that includes the changes needed for this feature will need to prereq the new v5 Go core. This would not mean, however, that a new major version of the SDK generator is required since the changes to the generated code will not constitute a breaking change for SDK users.

Before I do the steps above, I just wanted to double-check that the Cloudant team will, in fact, be able to consume these changes. IOW, would you be able to upgrade your minimum Go version to be 1.13+ as well? I suppose if you have not yet delivered a GA version of your SDK, that would be relatively easy to do. Otherwise it's a little more complicated :)

[padamstx]

I guess you can disregard my last post above. Just checked the cloudant go-sdk and you already list 1.13 as the min version of Go :)

[ricellis]

Declare a new major version (v5) of the Go core which supports Go 1.13+

I wish semver were a little more clear about this type of breaking dependency change, it says:

What should I do if I update my own dependencies without changing the public API?

That would be considered compatible since it does not affect the public API.
Software that explicitly depends on the same dependencies as your package
should have their own dependency specifications and the author will notice any conflicts.

Obviously someone taking the package update with an old version of Go would get breaks, my reading of that semver FAQ is that the expectation is that they should have their own minimum Go dependency declared and notice a conflict.

Just checked the cloudant go-sdk and you already list 1.13 as the min version

Indeed we do, but we're still in a pre 1.x position where we could make API changes to do the right thing.

Unless there is a strong objection to option 1, I think I'd prefer that option simply due to the fact that it would mean less disruption to our existing approach in Go generation.

Given that the major drawback of the other approaches (either splitting the request build/execute or adding parallel functions with a context arg) is introducing breaking API changes that would cause a major release then surely if the Go language bump causes a new major anyway I think we should reconsider whether taking this easier option is actually really any easier. Rather than rushing into a change here, perhaps we could consider gathering some more Go user feedback about what is a better API?

[padamstx]

Let me clarify...

The change in the Go core to require go 1.13+ (was go 1.12+) would cause a new major version of the Go core (v5), not the SDK generator (it's debatable whether the official semver rules would cause us to create a new core major version for a dependency upgrade, but our normal practice when upgrading minimum requirements for a core library is to declare a new major version to make it clear that something significant has changed, even if the core's "API" has not been changed in a breaking way).

We will deliver a new minor version of the SDK generator with the changes for this feature, and that new version will require the new v5.0.0 version of the Go core, but the SDK generator itself will not be generating new code that causes a breaking change. We typically try to avoid breaking changes in generated SDK code if at all possible.

So, I don't think this new revelation (need to take Go core to 5.0.0) really changes much in terms of our decision making as to how to implement the Context change.

Here's a more concrete example based on work I've done so far (Go core changes are finished, SDK generator changes are still in-progress but mostly finished).

1. The "options" struct for an operation will have a new field, like this:

   
   type MyOperationOptions struct {
       ...
   
   // An optional context.Context that will be associated with the http.Request
   // used to invoke the operation.  This can be used to specify a timeout/deadline,
   // or to cancel an in-flight request.
   requestContext *context.Context
   }

// SetRequestContext : Sets a context.Context instance to be used when invoking requests func (options ExerciseAuthOptions) SetRequestContext(param context.Context) ExerciseAuthOptions { options.requestContext = &param return options }

// GetRequestContext : Gets the context.Context instance func (options ExerciseAuthOptions) GetRequestContext() context.Context { return options.requestContext }

2. Here's an example of using a Context with an operation:

    ctx, _ := context.WithTimeout(context.Background(), 30 * time.Second)
    myOperationOptions := NewMyOperationOptions(<required parameters>)
    myOperationOptions.SetRequestContext(ctx)

    result, response, err := myService.MyOperation(myOperationOptions)

I've tested out this approach by hand-editing some generated unit tests and it all seems to work fine.

[padamstx]

Status: I've re-worked the Go core changes to avoid a new major version. I've also re-worked my SDK generator changes to use the dual-method approach. I'll use one of the cloudant operations to demonstrate how that works.

For the "getServerInformation" operation, we'll continue to generate the "GetServerInformation()" method with the same signature that it had before. The difference is that it is now just a wrapper around the new method "GetServerInformationWithContext()", like this:

func (ibmCloudant *IbmCloudantV0) GetServerInformation(getServerInformationOptions *GetServerInformationOptions) (result *ServerInformation, response *core.DetailedResponse, err error) {
    return ibmCloudant.GetServerInformationWithContext(context.Background(), getServerInformationOptions)
}

func (ibmCloudant *IbmCloudantV0) GetServerInformationWithContext(ctx context.Context, getServerInformationOptions *GetServerInformationOptions) (result *ServerInformation, response *core.DetailedResponse, err error) {
    err = core.ValidateStruct(getServerInformationOptions, "getServerInformationOptions")
    if err != nil {
        return
    }

    builder := core.NewRequestBuilder(core.GET)
    builder = builder.WithContext(ctx)

        ...

}

In addition, for any operation that returns any sort of response, the generated unit tests will include an additional "timeout" test where the "XXXWithContext()" method is used with a "timeout"-based Context instance.

I think I actually like this dual-method approach better than the "options model field" approach. I think I know what the Cloudant team's preference will be :)

[ibm-devx-automation]

:tada: This issue has been resolved in version 4.7.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

[padamstx]

Re-opening this after it was closed automatically due to the Go core PR being merged.

[padamstx]

This issue is now complete. The required go core changes are in go core v 4.7.0, and the changes in the Go generator are in v 3.15.0.