Commit ac3c698c authored by Tiago's avatar Tiago
Browse files

correctly clearing timers across the request lifecycle 

the previous commit was pointing at the right direction, as in, timers
were left dangling behind when requests were reset for retry. However,
not only they broke the total request timeout logic (which is supposed
to survive request resets), it also kept the timer reference with the
:idle timer (which would be dropped on reset, but isn't on request
completion).

this revamps the request timer clean up logic by making the request
store all request timers, which are then labelled. timers are cleared on
request reset AND request completion, where the total request timeout is
kept around on request reset specifically.
parent 3d35650c

lib/httpx/connection.rb

+3 −3
Original line number Diff line number Diff line
@@ -1120,14 +1120,14 @@ module HTTPX 
        end



        timer = selector.after(timeout, callback)

        request.active_timeouts << label

        request.once(:idle) { timer.cancel }

        timer.label = label

        request.active_timeouts << timer



        Array(finish_events).each do |event|

          # clean up request timeouts if the connection errors out

          request.set_timeout_callback(event) do

            timer.cancel

            request.active_timeouts.delete(label)

            request.active_timeouts.delete(timer)

          end

        end

      end

lib/httpx/request.rb

+18 −1
Original line number Diff line number Diff line
@@ -122,6 +122,7 @@ module HTTPX 


    def complete!(response = @response)

      emit(:complete, response)

      reset_timers(true)

    end



    # whether request has been buffered with a ping
@@ -297,7 +298,10 @@ module HTTPX 
        @body.rewind

        @ping = false

        @response = @drainer = nil

        @active_timeouts.clear



        # request may be sent to a different connection and will be

        # reassigned a new set of timers.

        reset_timers(false)

      when :headers

        return unless @state == :idle
@@ -361,6 +365,19 @@ module HTTPX 


      @on_response_arrived.call

    end



    private



    def reset_timers(reset_total_request_timers)

      timers = @active_timeouts



      until (timer = timers.shift).nil?

        next if !reset_total_request_timers && timer.label == :total_request_timeout



        # cancel active timers.

        timer.cancel

      end

    end

  end

end

lib/httpx/timers.rb

+3 −0
Original line number Diff line number Diff line
@@ -62,6 +62,9 @@ module HTTPX 
    end



    class Timer

      # simpler helper which allows classification

      attr_accessor :label



      def initialize(interval, callback)

        @interval = interval

        @callback = callback

regression_tests/bug_1_8_0_test.rb

+12 −10
Original line number Diff line number Diff line
@@ -18,19 +18,17 @@ class Bug_1_8_0_Test < Minitest::Test 
    with_unreliable_server do |uri|

      http = HTTPX.plugin(:persistent).with(timeout: { request_timeout: 10 })



      100.times do |i|

      100.times do

        http.get(uri).raise_for_status

      end



      selector = Thread.current.thread_variable_get(:httpx_persistent_selector_store)[http]



      callback_size = selector.

        instance_variable_get(:@timers).

        instance_variable_get(:@intervals).

        map { |interval| interval.instance_variable_get(:@callbacks).size }.

        sum

      callback_size = selector

                      .instance_variable_get(:@timers)

                      .instance_variable_get(:@intervals).sum { |interval| interval.instance_variable_get(:@callbacks).size }



      assert callback_size == 0, "Expected callbacks to be clear after close connection"

      assert callback_size.zero?, "Expected callbacks to be clear after close connection"

    end

  end
@@ -59,7 +57,11 @@ class Bug_1_8_0_Test < Minitest::Test 
    ensure

      begin

        Timeout.timeout(2) do

          server.close rescue nil

          begin

            server.close

          rescue StandardError

            nil

          end

        end

      rescue Timeout::Error

        thread.kill

sig/request.rbs

+2 −3
Original line number Diff line number Diff line
@@ -17,7 +17,7 @@ module HTTPX 
    attr_reader response: response?

    attr_reader http2_stream_options: { ?dependency: Integer, ?exclusive: bool, ?weight: Integer }

    attr_reader drain_error: StandardError?

    attr_reader active_timeouts: Array[Symbol]

    attr_reader active_timeouts: Array[Timers::Timer]



    attr_writer connection: Connection?

    attr_writer on_response_arrived: (^() -> void)?
@@ -93,7 +93,6 @@ module HTTPX 


    private



    def initialize_body: (Options options) -> Transcoder::_Encoder?



    def reset_timers: (bool reset_total_request_timers) -> void

  end

end