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

Allow end-user to reconfigure timeout on IRequest.Timeout #2112 #2379

Closed
wants to merge 1 commit into from

Conversation

tapika
Copy link

@tapika tapika commented Dec 3, 2021

Allow to set timeout for UploadAsset

@nickfloyd
Copy link
Contributor

Hey @tapika, thanks for the change here. I was wondering what you're specifically trying to accomplish here. The client has a pubic SetRequestTimeout on it that set's the timeout for the connection associated with the client.

Let me know, if you get the chance, I'd like to better understand the use case for the changeset. Thanks!

@tapika
Copy link
Author

tapika commented Jul 11, 2022

Hey @tapika, thanks for the change here. I was wondering what you're specifically trying to accomplish here. The client has a pubic SetRequestTimeout on it that set's the timeout for the connection associated with the client.

Let me know, if you get the chance, I'd like to better understand the use case for the changeset. Thanks!

As far as I remember - SetRequestTimeout did not work for me out of box. That's why I've wired this change.

@nickfloyd
Copy link
Contributor

As far as I remember - SetRequestTimeout did not work for me out of box. That's why I've wired this change.

Got it... I'll see if I can verify what you were seeing, especially given the previous implementation appeared that it would do the trick.

@nickfloyd
Copy link
Contributor

nickfloyd commented Jul 21, 2022

Hey @tapika,

I just verified this to be working in v1.0.0 of this SDK using:

var github = new GitHubClient(new ProductHeaderValue("TimeoutTest"));
var t = TimeSpan.FromMilliseconds(1);
github.SetRequestTimeout(t);
var user = await github.User.Get("nickfloyd");

It returns:

System.TimeoutException: A task was canceled.

A thing to note here, the timeout specified in the connection itself is only for the client connecting to the server. All this does is pass through the value you pass into System.Net.Http.HttpClient and assigns it to the Timeout property - which just sets the timespan to wait before the request itself times out.

Also, in the case where you might expect deadline propagation (meaning if the GitHub API makes other requests to other services the knowledge of the deadline is not propagated upsteam) to occur on the GitHub REST API side, it will not.

We need to correct the code docs for this API on the GitHubClient object. The code comments ("Set the GitHub Api request timeout.") implies that this will set the timeout for the GitHub API itself (which is capped at 10 seconds) and not the client timeout to the API.

@nickfloyd
Copy link
Contributor

I have added this as an up-for-grabs : #2506

I will close this PR based on the above - but if you feel like that was in error, please feel free to reopen or continue the discussion on the issue above.

Thank you for the contributions here; while this isn't what you probably expected it still helped to make the SDK better!

@nickfloyd nickfloyd closed this Jul 21, 2022
@nickfloyd nickfloyd added Type: Feature New feature or request Status: Invalid/Incomplete This doesn't seem right and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Invalid/Incomplete This doesn't seem right Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants