Skip to content

Conversation

haacked
Copy link
Contributor

@haacked haacked commented Aug 5, 2012

Hi all! It's me again.

The code I replaced was the following line:

http.Timeout = request.Timeout == 0 ? Timeout : request.Timeout;

Note that if request.Timeout and this.Timeout are both 0, this code would set http.Timeout to 0, overriding whatever value it might have been already.

What? "http.Timeout is by default 0 so this is a noop?" you say?

In the immortal words of Bart Simpson, au contraire, mon frère! You can't know that. RestSharp provides an extensibility point via the HttpFactory property of RestClient which allows you to substitute your own IHttpFactory.

This is how I found the issue because I overrode the IHttpFactory so that I could supply some defaults universally - whether I'm using an instance of RestClient or Http to make the request.

This pull request makes it so you simply don't set the property unless the Timeout of the request or the RestClient is greater than 0.

haacked added 2 commits August 4, 2012 20:50
The original code could override the http.Timeout with
the value of 0 if the RestClient Timeout property was 0.

If RestClient.Timeout <= 0, it shouldn't override the
http.Timeout.
If a user overrides the IHttpFactory, RestCLient shouldn't overwrite the
default user agent with its own unless both the RestClient and the Http
instance both have not set the user agent to a proper value.
@haacked
Copy link
Contributor Author

haacked commented Aug 5, 2012

Just added another commit to this PR. In this case, my user agent was being over-written by the RestClient with the RestSharp stuff. :(

This also has a slight optimization in that the old code was using reflection against the assembly every time a new instance was created to determine the version. That's not going to change over the lifetime of the AppDomain so calculate it once and be done. :)

petejohanson added a commit that referenced this pull request Aug 6, 2012
Fix problem with overriding request defaults
@petejohanson petejohanson merged commit 8763a56 into restsharp:master Aug 6, 2012
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.

2 participants