Stricter timeout options parsing

[stoivo]

fix #752

[stoivo]

What are we waiting for?

[tarcieri]

@stoivo the tests are red, for one thing

[stoivo]

I think this is a flaky spec

I will amend and push again just to check

[stoivo]

Pushed with no changed and now spec are green

[stoivo]

Very nice suggestion. So much more strait forward. It looks like it sneaked in a , in raise ArgumentError, "#{long} must be numeric", unless normalized[long].is_a?(Numeric)

After taking a closer look, I think we should do parsing a bit differently:

KEYS = %i[read write connect].to_h { |k| [k, :"#{k}_timeout"] }.freeze

class << self
  def normalize_options(options)
    normalized = {}
    original   = options.dup

    KEYS.each do |short, long|
      if original.key?(short) && original.key(long)
        raise ArgumentError, "can't pass both #{short} and #{long}"
      end

      normalized[long] = original.key?(long) ? original.delete(long) : original.delete(short)
      raise ArgumentError, "#{long} must be numeric", unless normalized[long].is_a?(Numeric)
    end

    if original.size.positive?
      raise ArgumentError, "unknown timeout options: #{original.keys.join(', ')}"
    end

    if normalized.empty?
      raise ArgumentError, "no timeout options given"
    end

    normalized
  end
end

You should be able to only set one of the actions callbacks like HTTP.timeout :read => 125?

[stoivo]

I like how it looks but rubocop this it is too high complexity. I think it's nice

normalize_options instead of parse_options

rubocop offences
be rubocop -a                                                                                                                                                                                                               15:37:07
lib/http/timeout/per_operation.rb:17:9: C: Metrics/AbcSize: Assignment Branch Condition size for normalize_options is too high. [<5, 23, 9> 25.2/17]
        def normalize_options(options) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/http/timeout/per_operation.rb:17:9: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for normalize_options is too high. [10/7]
        def normalize_options(options) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/http/timeout/per_operation.rb:17:9: C: Metrics/MethodLength: Method has too many lines. [11/10]
        def normalize_options(options) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/http/timeout/per_operation.rb:17:9: C: Metrics/PerceivedComplexity: Perceived complexity for normalize_options is too high. [10/8]
        def normalize_options(options) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 80/80 files 
80 files inspected, 4 offenses detected

[stoivo]

@ixti ping

[ixti]

oh. sorry, totally forgot. I'm afk till Monday, will merge as soon as get back

[ixti]

I've added a commit that disables inline some cops for that method, but I believe I found a regression. Right now it's possible to set global timeout with:

HTTP.timeout({ global: 161 })

And now that I'm looking at all of this again, I think next major we should add breaking change and:

• change #timeout signature to #timeout(type, ...)
• type will be one of: :none (or nil), :global, :per_operation
• signature of Global#initialize should be (value)
• signature of PerOperation#initialize should be (connect: 0, write: 0, read: 0)

@tarcieri WDYT?

[tarcieri]

Sounds fine to me although it would be a good time to consider potential other breaking changes too

[misalcedo]

I was comparing this HTTP client to others in Ruby and one thing I would like to add for timeouts is that global and per-operation are not mutually exclusive. I may want a request to time out after 1 minute, but spend no more than 150ms on the TCP connection portion or spend no more than 10 seconds idle on a read or write operation.

An alternative interpretation is what HTTPX does, which is to treat read as a timeout reading the response, write as a timeout writing the request and global as a overall timeout.

In my experience, except for streaming, HTTP clients care about total request timeout and maybe connect timeout, but definitely not per-operation timeouts. Streaming typically cares about operation timeouts and only optionally cares about a total timeout.

Often clients measure time to first byte from the server as well as overall latency, so timeouts on both of those could be helpful.

I am happy to help implement this type of behavior for 6.X if that is the target version.

[tarcieri]

@misalcedo I opened #773 to track a potential redesign of the timeout subsystem for http 6.x