Set the DefaultTimeout with the Connection

[shiftkey]

So #554 is one option for addressing the timeout issues in #567, but as @haacked and I discussed it'd be nicer to avoid the static property. So this is my proposal to satisfy that in the simplest possible way.

Connection is where we create the Request objects, before passing them to the HttpClientAdapter - so why not configure it there?

This adds a new ctor to GitHubClient where you can set this:

var client = new GitHubClient(new ProductHeaderValue("OctokitTests"), TimeSpan.FromMinutes(30));

I don't believe anyone else is using more than the default one, but I'm sure I'll hear about them sooner rather than later if they also want this...

• confirm via feedback that this approach suits the known issues/limitations

cc @distantcam @arakis for feedback

[shiftkey]

Putting this to [WIP] until I get feedback that this approach feels right...

[shiftkey]

I'm not hearing any objections from the crowd on this one? Is this thing on?

[haacked]

Should we allow changing it after the Connection has been created? For example, could it be a property (with a default value) so we don't need to add another ctor overload?

[distantcam]

Setting the timeout for all requests is ok, but I think it would be more useful to set a timeout for a specific upload call, as it may be significantly larger than the default for other calls.

Also, again specific to uploads, is it worth setting the timeout to null?

[shiftkey]

@haacked

Should we allow changing it after the Connection has been created?

As in overriding a hypothetical IConnection.Timeout property? Perhaps. That would be relatively easy to expose if you want to do things more granularly.

Setting the timeout for all requests is ok, but I think it would be more useful to set a timeout for a specific upload call, as it may be significantly larger than the default for other calls.

Sure, but what's the harm in doing this globally? I'd love to avoid over-complicating this API, I just wish I had a second example to go alongside uploading assets to say doing it globally is a saner approach.

Also, again specific to uploads, is it worth setting the timeout to null?

OK, this one interests me. HttpClient.Timeout isn't a nullable TimeSpan but you could just pass it TimeSpan.MaxValue and achieve the same effect...

Perhaps that's the compromise here, so you can do this sort of code:

var client = new GitHubClient(new ProductHeaderValue("my-cool-app"));
...
client.Connection.Timeout = TimeSpan.MaxValue;
var asset = await client.Release.UploadAsset(release, assetUpload);

@distantcam do you keep the GitHubClient around for multiple actions? Or would you dispose of it after the uploads are done?

[distantcam]

Typically we keep the client for a few actions, mostly create the draft release + upload assets to that draft.

Usually upload timeouts would be large, so setting it globally I don't want to wait 60 minutes to find out the release creation was the one that timed out. Being able to change the default between calls is ok, but it just feels a bit icky to me.

HttpClient.Timeout supports a value of Infinite so that would work instead of null.

Also, here's where I'm using Octokit. https://github.com/Particular/GitHubReleaseNotes/blob/master/src/App/Program.cs

[shiftkey]

@distantcam fair enough, I've got a couple of ideas for isolating it just to Releases. I'll see how they feel.

[shiftkey]

Alright, how does that test look and feel?

https://github.com/octokit/octokit.net/pull/587/files#diff-eafe4a5f92fff13edb4d8f5110616c9eR206

[distantcam]

That ContinueWith gives me the heebie jeebies.

[shiftkey]

That ContinueWith gives me the heebie jeebies.

Yeah, it's Not The Best. Open to other suggestions to contain that side-effect.

I don't want to be passing the timeout all the way through for the Post - perhaps there's some other pattern I can use here...

[shiftkey]

Ok, so I threw away all that previous work and went down the path of just making UploadAsset support overriding.

@haacked @distantcam let me know how that feels

[haacked]

Whitespace.

[shiftkey]

🐒

[distantcam]

The two SendData methods are very similar. I can see a modified-one-but-not-the-other error in your future.

[shiftkey]

Fiiiiiiine I'll merge those together better

[shiftkey]

🍎

[shiftkey]

Some of the integration tests are failing, but I'm gonna take this in because I no longer trust my CI setup 😢