Merge branch 'fix-pending' into 'master'

pending requests weren't being flushed to the write buffer.

See merge request honeyryderchuck/httpx!137

.rubocop.yml

@@ -6,6 +6,7 @@ AllCops:
   Include:
     - lib/**/*.rb
     - test/**/*.rb
+    - regression_tests/**/*.rb
     - integrations/**/*.rb
     - Rakefile
     - httpx.gemspec
@@ -91,3 +92,6 @@ Performance/TimesMap:
 Performance/RedundantBlockCall:
   Enabled: false
 
+Naming/ClassAndModuleCamelCase:
+  Exclude:
+    - regression_tests/**/*.rb

Rakefile

@@ -17,6 +17,13 @@ Rake::TestTask.new(:integrations) do |t|
   t.warning = false
 end
 
+desc "regression tests for particular incidents"
+Rake::TestTask.new(:regressions) do |t|
+  t.libs = %w[lib test]
+  t.pattern = "regression_tests/**/*_test.rb"
+  t.warning = false
+end
+
 RUBY_MAJOR_MINOR = RUBY_VERSION.split(/\./).first(2).join(".")
 
 begin

lib/httpx/connection.rb

@@ -362,6 +362,8 @@ module HTTPX
             write_drained = false
           end unless interests == :r
 
+          send_pending if @state == :open
+
           # return if socket is drained
           next unless (interests != :r || read_drained) &&
                       (interests != :w || write_drained)

lib/httpx/connection/http1.rb

@@ -44,6 +44,7 @@ module HTTPX
     def reset
       @max_requests = @options.max_requests || MAX_REQUESTS
       @parser.reset!
+      @handshake_completed = false
     end
 
     def close
@@ -264,7 +265,8 @@ module HTTPX
 
       requests_limit = [@max_requests, @requests.size].min
 
-      connection = if request.options.persistent || request != @requests[requests_limit - 1]
+      connection = if request != @requests[requests_limit - 1] &&
+                      request.options.persistent && @max_requests != 1
         "keep-alive"
       else
         "close"

regression_tests/bug_0_14_1_test.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "support/http_helpers"
+
+class Bug_0_14_1_Test < Minitest::Test
+  include HTTPHelpers
+
+  def test_multipart_can_have_arbitrary_conntent_type
+    uri = "https://#{httpbin}/post"
+
+    response = HTTPX.plugin(:multipart)
+                    .post(uri, form: {
+                            image: {
+                              content_type: "image/png",
+                              body: File.new(fixture_file_path),
+                            },
+                          })
+    verify_status(response, 200)
+    body = json_body(response)
+    verify_header(body["headers"], "Content-Type", "multipart/form-data")
+    # can't really test the filename, but if it's in the files field,
+    # then it was a file upload
+    verify_uploaded_image(body, "image", "image/png")
+  end
+
+  private
+
+  def origin(orig = httpbin)
+    "http://#{orig}"
+  end
+end

regression_tests/bug_0_14_2_test.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "support/http_helpers"
+
+class Bug_0_14_2_Test < Minitest::Test
+  include HTTPHelpers
+
+  def test_multipart_can_have_arbitrary_filename
+    uri = "https://#{httpbin}/post"
+
+    response = HTTPX.plugin(:multipart)
+                    .post(uri, form: {
+                            image: {
+                              filename: "weird-al-jankovic",
+                              body: File.new(fixture_file_path),
+                            },
+                          })
+    verify_status(response, 200)
+    body = json_body(response)
+    verify_header(body["headers"], "Content-Type", "multipart/form-data")
+    # can't really test the filename, but if it's in the files field,
+    # then it was a file upload
+    verify_uploaded_image(body, "image", "image/jpeg")
+  end
+
+  private
+
+  def origin(orig = httpbin)
+    "http://#{orig}"
+  end
+end

regression_tests/bug_0_14_3_test.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "support/http_helpers"
+
+class Bug_0_14_3_Test < Minitest::Test
+  include HTTPHelpers
+
+  def test_insecure_to_secure_redirect_was_carrying_connection_close_invalid_http2
+    insecure_uri = "http://www.nature.com/articles/nature10414"
+
+    session = HTTPX.plugin(:follow_redirects, max_redirects: 5)
+    redirect_response = session.get(insecure_uri)
+    verify_status(redirect_response, 200)
+    assert redirect_response.uri.scheme == "https"
+  end
+
+  private
+
+  def origin(orig = httpbin)
+    "http://#{orig}"
+  end
+end

regression_tests/bug_0_14_4_test.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "support/http_helpers"
+
+class Bug_0_14_4_Test < Minitest::Test
+  include HTTPHelpers
+
+  def test_http1_keep_alive_persistent_requests_failed_to_start_new_connection_after_max_requests_reached
+    server = KeepAliveServer.new
+    th = Thread.new { server.start }
+    begin
+      uri = "#{server.origin}/"
+      uris = [uri] * 400
+      HTTPX.plugin(SessionWithPool).with(max_requests: 100, max_concurrent_requests: 1).wrap do |http|
+        responses = http.get(*uris)
+        assert responses.size == 400
+        responses.each_with_index do |response, idx|
+          verify_status(response, 200)
+
+          conn_header = ((idx + 1) % 100).zero? ? "close" : "Keep-Alive"
+          assert verify_header(response.headers, "connection", conn_header)
+        end
+        connection_count = http.pool.connection_count
+        assert connection_count == 4, "expected to have 4 connections (+ an idle one), instead have #{connection_count}"
+      end
+    ensure
+      server.shutdown
+      th.join
+    end
+  end
+
+  def test_http1_keep_alive_persistent_requests_failed_to_start_new_connection_after_server_max_reached
+    server = KeepAliveServer.new
+    th = Thread.new { server.start }
+    begin
+      uri = "#{server.origin}/2"
+      uris = [uri] * 200
+      HTTPX.plugin(SessionWithPool).with(max_requests: 100, max_concurrent_requests: 1).wrap do |http|
+        responses = http.get(*uris)
+        assert responses.size == 200
+        responses.each_with_index do |response, idx|
+          verify_status(response, 200)
+          conn_header = ((idx + 1) % 2).zero? ? "close" : "Keep-Alive"
+          assert verify_header(response.headers, "connection", conn_header)
+        end
+        connection_count = http.pool.connection_count
+        assert connection_count == 100, "expected to have 100 connections (+ an idle one), instead have #{connection_count}"
+      end
+    ensure
+      server.shutdown
+      th.join
+    end
+  end
+end

regression_tests/bug_0_14_5_test.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "support/http_helpers"
+
+class Bug_0_14_5_Test < Minitest::Test
+  include HTTPHelpers
+
+  def test_http2_post_with_concurrent_post_requests_with_large_payload_blocking
+    post_uri = "https://#{httpbin}/post"
+    HTTPX.wrap do |http|
+      # this is necesary, so that handshake phase is complete
+      http.get("https://#{httpbin}/get")
+
+      requests = 2.times.map do
+        http.build_request(:post, post_uri, body: "a" * (1 << 16)) # 65k body, must be above write buffer size
+      end
+
+      responses = http.request(*requests)
+
+      responses.each do |response|
+        verify_status(response, 200)
+        body = json_body(response)
+        verify_header(body["headers"], "Content-Type", "application/octet-stream")
+        verify_uploaded(body, "data", "a" * (1 << 16))
+      end
+    end
+  rescue MinitestExtensions::TimeoutForTest::TestTimeout => e
+    ex = RegressionError.new(e.message)
+    ex.set_backtrace(e)
+    raise ex
+  end
+
+  private
+
+  def origin(orig = httpbin)
+    "http://#{orig}"
+  end
+end

test/http_test.rb

@@ -52,13 +52,12 @@ class HTTPTest < Minitest::Test
     server = KeepAliveServer.new
     th = Thread.new { server.start }
     begin
-      uri = "#{server.origin}/"
+      uri = "#{server.origin}/2"
       HTTPX.plugin(SessionWithPool).with(max_concurrent_requests: 1).wrap do |http|
         responses = http.get(uri, uri, uri)
         assert responses.size == 3, "expected 3 responses, got #{responses.size}"
         connection_count = http.pool.connection_count
         assert connection_count == 2, "expected to have 2 connections, instead have #{connection_count}"
-        assert http.connection_exausted, "expected 1 connnection to have exhausted"
       end
     ensure
       server.shutdown

test/support/assertion_helpers.rb

@@ -40,6 +40,11 @@ module ResponseHelpers
     assert_in_delta expected, actual, delta, "expected to have executed in #{expected} secs (actual: #{actual} secs)"
   end
 
+  def verify_uploaded(body, type, expect)
+    assert body.key?(type), "there is no #{type} available"
+    assert body[type] == expect, "#{type} is unexpected: #{body[type]} (expected: #{expect})"
+  end
+
   def verify_error_response(response, expectation = nil)
     assert response.is_a?(HTTPX::ErrorResponse), "expected an error response (instead got: #{response.inspect})"
 
@@ -59,4 +64,31 @@ module ResponseHelpers
       raise "unexpected expectation (#{expectation})"
     end
   end
+
+  # test files
+
+  def verify_uploaded_image(body, key, mime_type, skip_verify_data: false)
+    assert body.key?("files"), "there were no files uploaded"
+    assert body["files"].key?(key), "there is no image in the file"
+    # checking mime-type is a bit leaky, as httpbin displays the base64-encoded data
+    return if skip_verify_data
+
+    assert body["files"][key].start_with?("data:#{mime_type}"), "data was wrongly encoded (#{body["files"][key][0..64]})"
+  end
+
+  def fixture
+    File.read(fixture_file_path, encoding: Encoding::BINARY)
+  end
+
+  def fixture_name
+    File.basename(fixture_file_path)
+  end
+
+  def fixture_file_name
+    "image.jpg"
+  end
+
+  def fixture_file_path
+    File.join("test", "support", "fixtures", fixture_file_name)
+  end
 end

test/support/ci/build.sh

@@ -75,3 +75,11 @@ PARALLEL=1 bundle exec rake test:ci
 if [[ "$RUBY_ENGINE" = "ruby" ]]; then
   COVERAGE_KEY="#$RUBY_ENGINE-$RUBY_VERSION-integration" bundle exec rake integrations
 fi
+
+if [[ ${RUBY_VERSION:0:1} = "3" ]]; then
+  # regression tests
+  # Testing them only with main ruby
+  if [[ "$RUBY_ENGINE" = "ruby" ]]; then
+    COVERAGE_KEY="#$RUBY_ENGINE-$RUBY_VERSION-regression" bundle exec rake regressions
+  fi
+fi

test/support/requests/plugins/multipart.rb

@@ -219,31 +219,6 @@ module Requests
         assert check_error[retries_response], "expected #{retries_response} to be an error response"
         assert retries_session.calls == 1, "expect request to be retried 1 time (was #{retries_session.calls})"
       end
-
-      def fixture
-        File.read(fixture_file_path, encoding: Encoding::BINARY)
-      end
-
-      def fixture_name
-        File.basename(fixture_file_path)
-      end
-
-      def fixture_file_name
-        "image.jpg"
-      end
-
-      def fixture_file_path
-        File.join("test", "support", "fixtures", fixture_file_name)
-      end
-
-      def verify_uploaded_image(body, key, mime_type, skip_verify_data: false)
-        assert body.key?("files"), "there were no files uploaded"
-        assert body["files"].key?(key), "there is no image in the file"
-        # checking mime-type is a bit leaky, as httpbin displays the base64-encoded data
-        return if skip_verify_data
-
-        assert body["files"][key].start_with?("data:#{mime_type}"), "data was wrongly encoded (#{body["files"][key][0..64]})"
-      end
     end
   end
 end

test/support/requests/with_body.rb

@@ -133,12 +133,5 @@ module Requests
         end
       end
     end
-
-    private
-
-    def verify_uploaded(body, type, expect)
-      assert body.key?(type), "there is no #{type} available"
-      assert body[type] == expect, "#{type} is unexpected: #{body[type]} (expected: #{expect})"
-    end
   end
 end

test/support/servlets/keep_alive.rb

@@ -4,6 +4,15 @@ require_relative "test"
 
 class KeepAliveServer < TestServer
   class KeepAliveApp < WEBrick::HTTPServlet::AbstractServlet
+    def do_GET(_req, res) # rubocop:disable Naming/MethodName
+      res.status = 200
+      res["Connection"] = "Keep-Alive"
+      res["Content-Type"] = "application/json"
+      res.body = "{\"counter\": infinity}"
+    end
+  end
+
+  class KeepAliveMax2App < WEBrick::HTTPServlet::AbstractServlet
     def do_GET(_req, res) # rubocop:disable Naming/MethodName
       res.status = 200
       res["Connection"] = "Keep-Alive"
@@ -16,5 +25,6 @@ class KeepAliveServer < TestServer
   def initialize(options = {})
     super
     mount("/", KeepAliveApp)
+    mount("/2", KeepAliveMax2App)
   end
 end

test/test_helper.rb

@@ -38,3 +38,5 @@ end
 CONNECT_TIMEOUT_PORT = ENV.fetch("CONNECT_TIMEOUT_PORT", 9090).to_i
 ETIMEDOUT_PORT = ENV.fetch("ETIMEDOUT_PORT", 9091).to_i
 EHOSTUNREACH_HOST = ENV.fetch("EHOSTUNREACH_HOST", "192.168.2.1")
+
+RegressionError = Class.new(StandardError)