diff --git a/.rubocop.yml b/.rubocop.yml index 03d0cc83..a89d5d95 100644 --- a/.rubocop.yml +++ b/.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 \ No newline at end of file diff --git a/Rakefile b/Rakefile index 044594c4..d94e237d 100644 --- a/Rakefile +++ b/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 diff --git a/lib/httpx/connection.rb b/lib/httpx/connection.rb index 022d6116..63f6a0c3 100644 --- a/lib/httpx/connection.rb +++ b/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) diff --git a/lib/httpx/connection/http1.rb b/lib/httpx/connection/http1.rb index 297a7ed5..c3b2d96b 100644 --- a/lib/httpx/connection/http1.rb +++ b/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" diff --git a/regression_tests/bug_0_14_1_test.rb b/regression_tests/bug_0_14_1_test.rb new file mode 100644 index 00000000..3587a881 --- /dev/null +++ b/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 diff --git a/regression_tests/bug_0_14_2_test.rb b/regression_tests/bug_0_14_2_test.rb new file mode 100644 index 00000000..83788f14 --- /dev/null +++ b/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 diff --git a/regression_tests/bug_0_14_3_test.rb b/regression_tests/bug_0_14_3_test.rb new file mode 100644 index 00000000..704b5504 --- /dev/null +++ b/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 diff --git a/regression_tests/bug_0_14_4_test.rb b/regression_tests/bug_0_14_4_test.rb new file mode 100644 index 00000000..dd25f0c9 --- /dev/null +++ b/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 diff --git a/regression_tests/bug_0_14_5_test.rb b/regression_tests/bug_0_14_5_test.rb new file mode 100644 index 00000000..0d9b427d --- /dev/null +++ b/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 diff --git a/test/http_test.rb b/test/http_test.rb index 391aad90..a24f2af1 100644 --- a/test/http_test.rb +++ b/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 diff --git a/test/support/assertion_helpers.rb b/test/support/assertion_helpers.rb index 78e8a72c..491eede8 100644 --- a/test/support/assertion_helpers.rb +++ b/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 diff --git a/test/support/ci/build.sh b/test/support/ci/build.sh index b171e617..89d1b67a 100755 --- a/test/support/ci/build.sh +++ b/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 diff --git a/test/support/requests/plugins/multipart.rb b/test/support/requests/plugins/multipart.rb index 30631543..1c14e780 100644 --- a/test/support/requests/plugins/multipart.rb +++ b/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 diff --git a/test/support/requests/with_body.rb b/test/support/requests/with_body.rb index ab4e1d8c..76752b69 100644 --- a/test/support/requests/with_body.rb +++ b/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 diff --git a/test/support/servlets/keep_alive.rb b/test/support/servlets/keep_alive.rb index 33210c7c..86438c01 100644 --- a/test/support/servlets/keep_alive.rb +++ b/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 diff --git a/test/test_helper.rb b/test/test_helper.rb index dcbdfa7d..013b3af2 100644 --- a/test/test_helper.rb +++ b/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)