improve the story about controlling request timeouts

[shiftkey]

trafficstars

Opening this to track making this easier to understand and eliminate confusion about HttpClient global and per-Request timeouts (the lower of the two is what will be used).

• [ ] share some code samples where timeouts matter
• [ ] ???

Issues in the wild that may be relevant: #2045 #1069 #984

[shiftkey]

Currently there are three areas where timeouts can be changed:

• GitHubClient.SetRequestTimeout to set the timeout globally

var client = new GitHubClient(new Connection(new ProductHeaderValue("OctokitTests")));
client.SetRequestTimeout(TimeSpan.FromSeconds(15));

• IRequest.Timeout is set per-request, and the constructor for Request sets a default timeout of 100 seconds.
• ReleaseAssetUpload.Timeout has a nullable Timespan? property that can be used to set a higher time.

One of the known issues here is where a global and per-request timeout are specified, the lower value is the winner. This is likely a source of confusion for users who set a higher value globally but can't access the inner default 100s on Request.

[arxange1]

Lets double check the purpose of every timeout field we have:

1. HttpClient.Timeout in the field HttpClientAdapter._http

• Scope: per-request timeout
• In use by: HttpClient internaly
• How can be changed by library user: via SetRequestTimeout() method
• How can be read for diagnostics: not implemented

2. IRequest.Timeout (100 seconds)

• Scope: global timeout
• In use by: HttpClientAdapter.GetCancellationTokenForRequest() method only
• How can be changed by library user: not implemented
• But: Implemented for a few methods like GetArchive(..., timeout)
• How can be read for diagnostics: not implemented

[shiftkey]

@arxange1 isn't 1) the global timeout and 2) the per-request timeout?

[arxange1]

@shiftkey To get a right answer we have to define what the global timeout is. Is pagination included in global scope?

[arxange1]

According to docs.microsoft.com both are per-request. The difference is that the first will be applied to all requests and the send one to individual request. But the Octokit passes individual 100s timeout to all requests, so the Timeout property one does not matter, still not less than 100s.

I confused the definition of "global". It means system-wide, I thought operation-wide

[shiftkey]

To get a right answer we have to define what the global timeout is.

I don't quite understand, but I want to focus on what Octokit should be doing, rather than what it's currently doing.

I'm considering HttpClient.Timeout to be a global setting based on this sentence from the docs (emphasis mine).

The same timeout will apply for all requests using this HttpClient instance.

Is pagination included in global scope?

We can talk about pagination, but I think it's just a different "per-request" situation. A request gets completed, it tries to follow the pagination headers, and if they are found it'll create a new request.

[arxange1]

I see, so the picture is that we have no general approach to influence individual per-request timeout which is defined in the field of internal class Request.Timeout. There are a few exposed API method overloads which includes "timeout" parameter (like GetArchive), but that's all we have for now.

[shiftkey]

@arxange1 correct - at the time we didn't want to make users really think about that, which is why we provided those specific overloads as well as the global GitHubClient.SetRequestTimeout.

[arxange1]

share some code samples where timeouts matter

Consider this scenario: We handle transient errors using a custom HttpClientHandler. In that case "individual per-request" timeout includes retry attempts. It is In order to keep a tight control over timeouts we have to distinguish actual per-request timeout and CancellationToken timeout that is passed to HttpClient.SendAsync() which is span across all retries. But the scenario is impossible to implement still the CancellationToken timeout is predefined and always 100 seconds.

[csmager]

ReleaseAssetUpload.Timeout has a nullable Timespan? property that can be used to set a higher time.

Though I think this is implied, just for clarity (as it surprised me): if you leave this as null then the default of 100 seconds is applied.

I think I expected no per-request timeout to be set in this case, and that it would just be ended by the 'global' timeout set on HttpClient. As it is, you need to remember to set a larger timeout globally and on ReleaseAssetUpload.Timeout.

[antichaosdb]

I'm getting 100s timeouts on downloading large assets using Connection.Get<Byte[]> GitHubClient.SetRequestTimeout is set at 600 seconds. I've tried passing a CancellationToken with a larger timeout too.

[github-actions[bot]]

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!