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

Provided a httpclient timeout option #965

Closed

Conversation

naveensrinivasan
Copy link

The HttpClient does not take the Timeout argument which by default is 100 seconds. Implemented a change where the user has an option to provide the timeout. This is to address the #963.

The `HttpClient` does not take the `Timeout` argument which by default
is 100 seconds. Implemented a change where the user has an option to
provide the timeout. This is to address the octokit#963.
@shiftkey
Copy link
Member

Yeah, this is something I'd like to make available to consumers - I've just been grappling with how to get there due to how we're currently using HttpClient - see #781 for all the details.

Your solution highlights the pain we're currently in with respect to overloads to GitHubClient, and most of the ctor parameters can actually be used for HttpClient. I don't feel great about layering more on there, but perhaps that's a reasonable short-term fix.

It's been almost six months since I last thought about this, so I'll go back to this and revisit that topic.

@naveensrinivasan
Copy link
Author

Wasn't aware of that.

@@ -130,6 +131,10 @@ public static IGitHubClient GetAnonymousClient()
{
return new GitHubClient(new ProductHeaderValue("OctokitTests"));
}
public static IGitHubClient GetAnonymousClient(TimeSpan httpTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate methods with newlines.

Naveen added 4 commits November 30, 2015 20:32
Included a line break in between methods.
Removed the additional line break.
Included a line break in between methods.
Removed the additional line break.
@naveensrinivasan
Copy link
Author

Fixed the line breaks and spaces. @shiftkey Will the CodeFormatter address these line breaks and spaces?

@shiftkey
Copy link
Member

shiftkey commented Dec 1, 2015

@naveensrinivasan @haacked yes, if you're on VS2015 you should be able to run it from the command line: .\build FormatCode

If that's not working As Advertised™, I can investigate further.

@naveensrinivasan
Copy link
Author

When I branched the VS2015 branch wasn't available. Will do that going forward. Thanks!

@shiftkey
Copy link
Member

shiftkey commented Dec 8, 2015

@naveensrinivasan first off, thanks for working on this. I should have made my reservations about adding another ctor parameter clearer, as I'd been trying to think of a better solution to this problem but was sidetracked as usual.

I'm not enthusiastic about adding in yet another ctor parameter to GitHubClient - mostly due to the current complexity of how timeouts work with the HTTP stack, which you've called out elsewhere. The other concern I have here is about supporting this long-term - at some point I'd love to support a smaller set of ctors on GitHubClient, but that's a debate that has yet to happen so I'm just speaking for myself right now.

So, while I feel awful for closing this PR, I'm trying to make amends by opening a discussion about making the whole HTTP stack more friendly to Octokit consumers #984. I've also opened the first PR which takes us down that path #985 so we can talk about tangible things. I'd love your feedback on either (or both!) topic, as I don't think there's as much work involved as I previously expected - we just need to sit down and work through it...

@shiftkey shiftkey closed this Dec 8, 2015
@naveensrinivasan
Copy link
Author

@shiftkey Not an issue. 👍

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.

3 participants