Allow to define open and read timeout individualy

[ceritium]

The current implementation uses the argument timeout to define both open and read timeout; therefore, for a timeout of 30 seconds, the query will have a total timeout of 60 seconds. Also, it's common to want to define a lower open timeout but allow larger read timeouts.

Closes: https://github.com/ruby/xmlrpc/issues/45

[herwinw]

There is a bit of inconsistent behaviour in this pull request. A snippet of the old/unchanged timeout setter:

attr_reader :timeout

def timeout=(new_timeout)
  @timeout = new_timeout
  @http.read_timeout = @timeout
  @http.open_timeout = @timeout
end

The @timeout instance variable is not updated with these new setters, which would mean:

client = XMLRPC::Client.new(...)
client.timeout = 30
p client.timeout # => 30
client.open_timeout = 40
client.read_timeout = 40
p client.timeout # => 30, i would expect this to print 40 instead
client.open_timeout = 50
p client.timeout # => 30, so now we've got 3 different timeout values

The easiest solution would be to get rid of the version ivar/reader, and replace them with ivars/readers for both new timeouts. This does break backwards compatibility though.

[kou]

How about accepting [read_timeout, open_timeout] as timeout too? XMLRPC::Client#timeout returns timeout only if read_timeout and open_timeout are the same value. If they are different values, it returns [read_timeout, open_timeout]. The current all user codes use an integer for timeout. No codes that use [read_timeout, open_timeout]. So it doesn't break backward compatibility.

client = XMLRPC::Client.new(...)
client.timeout = 30
p client.timeout # => 30
client.timeout = [40, 50] # New behavior
p client.timeout # => [40, 50] # New behavior but existing codes never get this
p client.read_timeout # => 40
p client.open_timeout # => 50
client.open_timeout = 40
p client.timeout # => 40

[ceritium]

Hi, I would remove the ivars and store the values directly on @http.

@kou, I like your suggestion, except for client.timeout = [40, 50], we can define them with their specific methods #open_timeout= and #read_timeout=.

#timeout can return an array with open and read timeout only if the values differ. It will keep the backwards compatibility and only introduce a new kind of value for those integrations using the new methods.

Does it make sense for you both?

[herwinw]

That solution is not 100% backwards compatible, there is still a chance that some code changes the open/read timeout directly on the http object (even though the docs say not to do that)

# Returns the Net::HTTP object for the client. If you want to
# change HTTP client options except header, cookie, timeout,
# user and password, use Net::HTTP directly.
#
# Since 2.1.0.
attr_reader :http

Also, I might be wrong here, but I assume the read timeout check starts after the open operation. So when I set client.timeout = 30, I'm actually setting both the open and the read timeout to 30, so I could get really unlucky with an open operation that takes 29 seconds and a read operations that takes another 29 seconds. It's kind of weird that you can set a timeout and have a request that takes twice that time still be valid. Just to be clear, this is an issue with the current code as well, I simply never noticed this before.

More of a personal preference, but I dislike methods that can return multiple types. Imagine the use case where the open and read timeouts are set based on something external (like a config file), and my code calling Client#timeout has to check if the return value is an Int or an Array. But then again, if I cared about separate values for open and read timeout, I probably wouldn't be calling Client#timeout, so I don't think this is much of an issue.

So I actually think that the timeout property should be deprecated.

[ceritium]

Also, I might be wrong here, but I assume the read timeout check starts after the open operation. So when I set client.timeout = 30, I'm actually setting both the open and the read timeout to 30, so I could get really unlucky with an open operation that takes 29 seconds and a read operations that takes another 29 seconds. It's kind of weird that you can set a timeout and have a request that takes twice that time still be valid. Just to be clear, this is an issue with the current code as well, I simply never noticed this before.

Yes, that's one of the reasons why I want to set the open and read timeouts individually.

More of a personal preference, but I dislike methods that can return multiple types.

I agree

So I actually think that the timeout property should be deprecated.

I think it would be the best. I am unfamiliar with this project and don't know the procedure for deprecations. What is your proposal?

• How can we mark it as deprecated?
• When would be definitively removed?
• Should we allow to set the open and read timeouts from the initializers?j