Skip to content
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

feat: Add option to pass external contexts into Go SDK's HTTP calls #1475

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

JCPistell
Copy link
Contributor

This adds the option to include an external context in the Go SDK's HTTP calls. This is useful if the SDK is being used as part of a larger application that needs to share a context across multiple API boundaries.

@JCPistell JCPistell requested a review from a team as a code owner June 20, 2024 18:50
Copy link
Contributor

github-actions bot commented Jun 20, 2024

Go Tests

  6 files    6 suites   3m 35s ⏱️
 55 tests  55 ✅ 0 💤 0 ❌
130 runs  130 ✅ 0 💤 0 ❌

Results for commit 81ac004.

♻️ This comment has been updated with latest results.

@jkaster jkaster requested a review from jeremytchang June 20, 2024 18:57
Copy link
Collaborator

@jeremytchang jeremytchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition!
I left a few comments on updating documentation and comments to make clear what the behavior/side effects will be when setting context on per or all requests. Also had a small ask around increased testin.

go/README.md Outdated Show resolved Hide resolved
go/rtl/auth.go Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/rtl/auth.go Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/rtl/auth_test.go Show resolved Hide resolved
Copy link
Collaborator

@jeremytchang jeremytchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of my comments! And your documentation looks great! LGTM

Copy link
Collaborator

@jeremytchang jeremytchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking for now as there's a proposal that would supercede this.

Copy link
Contributor

APIX Tests

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 57fe967.

Copy link
Contributor

Python Tests

  9 files    9 suites   1m 17s ⏱️
144 tests 139 ✅  5 💤 0 ❌
676 runs  657 ✅ 19 💤 0 ❌

Results for commit 57fe967.

Copy link
Contributor

Codegen Tests

418 tests   400 ✅  1m 6s ⏱️
 18 suites   18 💤
  1 files      0 ❌

Results for commit 57fe967.

Copy link
Contributor

Typescript Tests

 10 files   64 suites   5m 49s ⏱️
227 tests 223 ✅  4 💤 0 ❌
656 runs  624 ✅ 32 💤 0 ❌

Results for commit 57fe967.

if s.Config.Timeout != 0 {
timeoutInSeconds = s.Config.Timeout
}
if options != nil && options.Timeout != 0 {
timeoutInSeconds = options.Timeout
}

ctx, cncl := context.WithTimeout(context.Background(), time.Second*time.Duration(timeoutInSeconds))
ctx, cncl := context.WithTimeout(parent, time.Second*time.Duration(timeoutInSeconds))
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the parent context has a different timeout, what's the expected behavior here? Does it go with the shorter one?

Copy link
Contributor Author

@JCPistell JCPistell Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. If the parent hits its cancellation condition (e.g. a timeout) first then the cancellation propagates to the child.

However, If the child cancels first, it depends on how that specific error is handled... perhaps the request retries, perhaps the error is passed to the calling function, in which case most of the time the function fails and the parent context then cancels, which propagates the cancellation to all other goroutines, etc etc.

Basically this approach means you can have an overall timeout set by a parent, but each individual request running in a goroutine can also have its own timeout... this avoids the scenario where you set a 60min overall timeout, but one bad request has to wait the entire 60m to time out and fail.

if s.Config.Timeout != 0 {
timeoutInSeconds = s.Config.Timeout
}
if options != nil && options.Timeout != 0 {
timeoutInSeconds = options.Timeout
}

ctx, cncl := context.WithTimeout(context.Background(), time.Second*time.Duration(timeoutInSeconds))
ctx, cncl := context.WithTimeout(parent, time.Second*time.Duration(timeoutInSeconds))
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an easier to understand behavior would be:
If you use config's context or option's context, the timeout defined in options and config is ignored.

Trying to document/understad the how timeout works with context/config/options is confusing and harder to test. Plus if dev is using contexts, they are a power user who has way more levers to pull, and should know about using timeouts in contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree - see my previous comment. I actually think this is easier to document and the tests are already in place.


#### Custom Context per request

Follow the example here to set a context for a specific request.
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add "For the specific request, the custom context will replace the custom context set for all requests in the previous "Custom Context for all requests" section."

}
```

> Note: Setting a context per request will NOT affect the context used for the background token fetching requests. If you have also set a context for all requests as mentioned above then that context
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add "If you set a custom context for a specific request, the SDK will ignore any timeout described in the previous Timeout section for the specific request. You must set the timeout with the custom context for the specific request."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't true - request timeouts will still apply - any specified context simply becomes the parent of the timeout request.


If you want even greater control over the lifecycle of the HTTP requests consider providing a [context](https://pkg.go.dev/context). Contexts can be set for all requests or per request. They can be combined with the timeout options mentioned in the previous section to fine-tune the lifecycle of your requests.

If you wish to include timeout functionality in your custom context then you should leverage [context.WithTimeout](https://pkg.go.dev/context#WithTimeout).
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to:

The SDK will ignore any timeout described in the previous Timeout section for all requests if you use custom context for all requests, or per request if you use custom context per request.

You must use [context.WithTimeout](https://pkg.go.dev/context#WithTimeout) functionality to set a timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comment - this is inaccurate.


If you wish to include timeout functionality in your custom context then you should leverage [context.WithTimeout](https://pkg.go.dev/context#WithTimeout).

> Note: Custom contexts will be used as the parent context for any timeout you set as specified in the previous section. If the parent context gets cancelled it will propagate to the child context, but if the timeout context times out it does not propagate to the parent context.
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this means? Can we remove this if we're going with the new behavior?

@@ -118,3 +118,57 @@ func main() {
}
}
```
### Custom Context

If you want even greater control over the lifecycle of the HTTP requests consider providing a [context](https://pkg.go.dev/context). Contexts can be set for all requests or per request. They can be combined with the timeout options mentioned in the previous section to fine-tune the lifecycle of your requests.
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to:

You can set a [context](https://pkg.go.dev/context) for all requests or a specific request for more control over the SDK's HTTP requests.

}
```

> Note: A context set here will become the parent context for all API calls as well as all requests to fetch/refresh oauth tokens, which are normally completely isolated from contexts set via the Timeout property. In this case the token refresh requests and each individual API call will share a common parent context.
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to

Note: The SDK will ignore any timeout described in the previous Timeout section if you use custom context for all requests. You must set the timeout with the custom context.

Note: The context set here will become the parent context for all requests including the SDK's background oauth token requests. If you want to control the timeout of the SDK's background oauth token requests, you must set a custom context (with a timeout) for all requests.```


Follow the example here to set a context for a specific request.

> Note: This will be used as the parent context for any timeout setting you've specified for API calls. If you've set contexts in both your API config and in the request options the request options context will be used instead. Background requests to fetch/refresh oauth tokens will NOT use a context set via request options - it will default to use a generic background context or, if you've also set a context in the API config it will still use that as specified in the previous section.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed

}
```

> Note: Setting a context per request will NOT affect the context used for the background token fetching requests. If you have also set a context for all requests as mentioned above then that context
Copy link
Collaborator

@jeremytchang jeremytchang Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with

Note: If you set a custom context for a specific request, the SDK will ignore any timeout described in the previous Timeout section for the specific request. You must set the timeout with the request's custom context.

Note:
The custom context per request does NOT affect the background oauth token requests' context. You must use a custom context set on all requests to do so.

@drstrangelooker
Copy link
Collaborator

@JCPistell has this been superseded? Can we close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants