-
Notifications
You must be signed in to change notification settings - Fork 24
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
Support for context timeout/deadline for requests in underlying HTTP client #77
Comments
I'm going to transfer this to https://github.com/IBM/go-sdk-core so it can be taken under consideration there. |
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 would make sense if a Context is more closely related to a particular request. I think I'm leaning toward #1. Comments? |
I agree with this sentiment. However, the Go docs for
So I think perhaps the Go way to do this would be to either generate the additional functions with the However, I do think the option 1 approach of having the |
@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:
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 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:
cc: @mkistler |
I understand, I read it as two separate reasons:
I agree that neither of those are as stark as something suggesting it a 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. |
Ok, so we have two different options:
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. |
Fixes arf/planning-sdk-squad#2230 Fixes #77 This commit adds support for setting a context.Context instance on the RequestBuilder struct. This Context instance will then be associated with the http.Request instance constructed by the RequestBuilder.Build() method.
Status: |
@ricellis @alexsniffin This means that I'll need to do the following in order to complete this feature:
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 :) |
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 :) |
Fixes arf/planning-sdk-squad#2230 Fixes #77 This commit adds support for setting a context.Context instance on the RequestBuilder struct. This Context instance will then be associated with the http.Request instance constructed by the RequestBuilder.Build() method. BREAKING CHANGE: Minimum supported Go version is now 1.13. The minimum version of Go supported by this library has been changed to Go version 1.13. To use this and future versions of the Go core library, please make sure that you are using Go version 1.13 or higher.
Fixes arf/planning-sdk-squad#2230 Fixes #77 This commit adds support for setting a context.Context instance on the RequestBuilder struct. This Context instance will then be associated with the http.Request instance constructed by the RequestBuilder.Build() method. BREAKING CHANGE: Minimum supported Go version is now 1.13. The minimum version of Go supported by this library has been changed to Go version 1.13. To use this and future versions of the Go core library, please make sure that you are using Go version 1.13 or higher.
I wish semver were a little more clear about this type of breaking dependency change, it says:
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.
Indeed we do, but we're still in a pre 1.x position where we could make API changes to do the right thing.
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? |
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).
I've tested out this approach by hand-editing some generated unit tests and it all seems to work fine. |
Fixes arf/planning-sdk-squad#2230 Fixes #77 This commit adds support for setting a Context instance on the RequestBuilder struct. This Context instance will then be associated with the http.Request instance constructed by the RequestBuilder.Build() method. The Context can be used to define a timeout/deadline for a request or to cancel an in-flight request.
Status: 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:
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 :) |
Fixes arf/planning-sdk-squad#2230 Fixes #77 This commit adds support for setting a Context instance on the RequestBuilder struct. This Context instance will then be associated with the http.Request instance constructed by the RequestBuilder.Build() method. The Context can be used to define a timeout/deadline for a request or to cancel an in-flight request.
# [4.7.0](v4.6.1...v4.7.0) (2020-10-15) ### Features * support use of Context with RequestBuilder ([d8e3f71](d8e3f71)), closes [arf/planning-sdk-squad#2230](https://github.com/arf/planning-sdk-squad/issues/2230) [#77](#77)
🎉 This issue has been resolved in version 4.7.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Re-opening this after it was closed automatically due to the Go core PR being merged. |
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. |
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.:
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.
The text was updated successfully, but these errors were encountered: