Stricter timeout options parsing #754

stoivo · 2023-06-08T09:06:37Z

Move option transformation into then block since we want to extent it.

lib/http/timeout/per_operation.rb

tycooon · 2023-06-09T06:39:57Z

lib/http/chainable.rb

@@ -90,21 +90,18 @@ def build_request(*args)
    # @overload timeout(global_timeout)
    #   Adds a global timeout to the full request
    #   @param [Numeric] global_timeout
+    PER_OPERATION_KEYS = Set.new(%i[read write connect])


This looks unused :)

moved in into lib/http/timeout/per_operation.rb but didn't remove it from here

stoivo · 2023-06-16T06:27:03Z

What are we waiting for?

tarcieri · 2023-06-16T12:20:52Z

@stoivo the tests are red, for one thing

stoivo · 2023-06-19T06:30:49Z

I think this is a flaky spec

I will amend and push again just to check

Rubocop told me the method started to be too complex. lib/http/chainable.rb:94:5: C: Metrics/PerceivedComplexity: Perceived complexity for timeout is too high. [10/8] def timeout(options) ... ^^^^^^^^^^^^^^^^^^^^ 80 files inspected, 1 offense detected

stoivo · 2023-06-19T06:41:38Z

Pushed with no changed and now spec are green

ixti

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

ixti · 2023-06-19T18:30:05Z

lib/http/timeout/per_operation.rb

+            next unless options.key? k
+
+            if options.key?("#{k}_timeout".to_sym)
+              raise ArgumentError, "can't pass both #{k} and #{"#{k}_timeout".to_sym}"


Suggested change

raise ArgumentError, "can't pass both #{k} and #{"#{k}_timeout".to_sym}"

raise ArgumentError, "can't pass both #{k} and #{k}_timeout"

ixti · 2023-06-19T18:30:40Z

lib/http/timeout/per_operation.rb

+
+      class << self
+        def parse_options(options)
+          options = options.dup.then { |opts| expand_names(opts) }


Suggested change

options = options.dup.then { |opts| expand_names(opts) }

options = expand_names(options.dup)

ixti · 2023-06-19T18:33:00Z

lib/http/timeout/per_operation.rb

+          options.each do |key, value|
+            unless SETTINGS.member?(key) && value.is_a?(Numeric)
+              raise ArgumentError, "invalid option #{key.inspect}, must be numeric " \
+                                   "`.timeout(connect: x, write: y, read: z)`."
+            end
+          end
+
+          raise ArgumentError, "at least one option" if options.empty?
+
+          options


I believe we also should fail if given options are mistyped or totally wrong ones

this is covered by

options.each do |key, value| unless SETTINGS.member?(key) && value.is_a?(Numeric) raise ArgumentError, "invalid option #{key.inspect}, must be numeric " \ "`.timeout(connect: x, write: y, read: z)`." end end

stoivo · 2023-06-26T13:22:46Z

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?

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 |================================================================================================================ 100 =================================================================================================================>| Time: 00:00:00 80 files inspected, 4 offenses detected

stoivo · 2023-06-26T14:01:03Z

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 · 2023-07-07T08:12:03Z

@ixti ping

ixti · 2023-07-07T12:15:03Z

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

ixti · 2023-07-14T20:04:36Z

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 · 2023-07-14T20:10:50Z

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

misalcedo · 2023-12-13T13:56:23Z

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 · 2023-12-13T14:48:16Z

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

Reorganize chainable timeout

4e6d386

Move option transformation into then block since we want to extent it.

ixti requested changes Jun 8, 2023

View reviewed changes

lib/http/timeout/per_operation.rb Show resolved Hide resolved

lib/http/timeout/per_operation.rb Outdated Show resolved Hide resolved

stoivo force-pushed the stricter_timeout branch from 618641e to 22326db Compare June 9, 2023 06:37

tycooon reviewed Jun 9, 2023

View reviewed changes

Be stricter of allowed values for timeout

0b150cb

stoivo force-pushed the stricter_timeout branch from 22326db to aa9451f Compare June 9, 2023 07:46

Abstract timeout option parsing

46b8aeb

Rubocop told me the method started to be too complex. lib/http/chainable.rb:94:5: C: Metrics/PerceivedComplexity: Perceived complexity for timeout is too high. [10/8] def timeout(options) ... ^^^^^^^^^^^^^^^^^^^^ 80 files inspected, 1 offense detected

stoivo force-pushed the stricter_timeout branch from aa9451f to 46b8aeb Compare June 19, 2023 06:33

ixti requested changes Jun 19, 2023

View reviewed changes

Update rubbocop

11defe7

Downgrade rubocop

ccf790e

tarcieri mentioned this pull request Dec 13, 2023

timeout: undefined method `to_sym' for :Integer #603

Closed

misalcedo mentioned this pull request Dec 13, 2023

Timeout redesign for 6.x #773

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stricter timeout options parsing #754

Stricter timeout options parsing #754

stoivo commented Jun 8, 2023

tycooon Jun 9, 2023

stoivo Jun 9, 2023

stoivo commented Jun 16, 2023

tarcieri commented Jun 16, 2023

stoivo commented Jun 19, 2023

stoivo commented Jun 19, 2023

ixti left a comment

ixti Jun 19, 2023

ixti Jun 19, 2023

ixti Jun 19, 2023

stoivo Jun 26, 2023 •

edited

stoivo commented Jun 26, 2023

stoivo commented Jun 26, 2023

stoivo commented Jul 7, 2023

ixti commented Jul 7, 2023

ixti commented Jul 14, 2023 •

edited

tarcieri commented Jul 14, 2023

misalcedo commented Dec 13, 2023 •

edited

tarcieri commented Dec 13, 2023

	raise ArgumentError, "can't pass both #{k} and #{"#{k}_timeout".to_sym}"
	raise ArgumentError, "can't pass both #{k} and #{k}_timeout"

	options = options.dup.then { \|opts\| expand_names(opts) }
	options = expand_names(options.dup)

Stricter timeout options parsing #754

Are you sure you want to change the base?

Stricter timeout options parsing #754

Conversation

stoivo commented Jun 8, 2023

tycooon Jun 9, 2023

Choose a reason for hiding this comment

stoivo Jun 9, 2023

Choose a reason for hiding this comment

stoivo commented Jun 16, 2023

tarcieri commented Jun 16, 2023

stoivo commented Jun 19, 2023

stoivo commented Jun 19, 2023

ixti left a comment

Choose a reason for hiding this comment

ixti Jun 19, 2023

Choose a reason for hiding this comment

ixti Jun 19, 2023

Choose a reason for hiding this comment

ixti Jun 19, 2023

Choose a reason for hiding this comment

stoivo Jun 26, 2023 • edited

Choose a reason for hiding this comment

stoivo commented Jun 26, 2023

stoivo commented Jun 26, 2023

stoivo commented Jul 7, 2023

ixti commented Jul 7, 2023

ixti commented Jul 14, 2023 • edited

tarcieri commented Jul 14, 2023

misalcedo commented Dec 13, 2023 • edited

tarcieri commented Dec 13, 2023

stoivo Jun 26, 2023 •

edited

ixti commented Jul 14, 2023 •

edited

misalcedo commented Dec 13, 2023 •

edited