From 8e9538a524f174f7406f7cc50ab762b24fd46bd2 Mon Sep 17 00:00:00 2001 From: Erik Berlin Date: Mon, 9 Mar 2026 02:44:46 -0700 Subject: [PATCH 1/2] Introduce HTTP::Session for thread-safe request building Chainable option methods (headers, timeout, cookies, etc.) now return an HTTP::Session instead of an HTTP::Client. Session is a lightweight, thread-safe builder that creates a fresh Client for each request, fixing the thread-safety issue where sharing a configured client across threads caused IOError due to shared mutable connection state. HTTP.retriable now returns an HTTP::Retriable::Session. HTTP.persistent still returns an HTTP::Client since persistent connections require mutable state. Closes https://github.com/httprb/http/issues/306. --- .rubocop/style.yml | 2 + CHANGELOG.md | 11 ++ README.md | 28 +++++ lib/http.rb | 6 +- lib/http/chainable.rb | 167 ++++++---------------------- lib/http/chainable/verbs.rb | 127 +++++++++++++++++++++ lib/http/retriable/client.rb | 13 ++- lib/http/retriable/session.rb | 51 +++++++++ lib/http/session.rb | 82 ++++++++++++++ sig/http.rbs | 81 ++++++++++---- test/http/retriable/client_test.rb | 24 +++- test/http/retriable/session_test.rb | 38 +++++++ test/http/session_test.rb | 151 +++++++++++++++++++++++++ test/http_test.rb | 6 +- 14 files changed, 615 insertions(+), 172 deletions(-) create mode 100644 lib/http/chainable/verbs.rb create mode 100644 lib/http/retriable/session.rb create mode 100644 lib/http/session.rb create mode 100644 test/http/retriable/session_test.rb create mode 100644 test/http/session_test.rb diff --git a/.rubocop/style.yml b/.rubocop/style.yml index 408d6792..dc430871 100644 --- a/.rubocop/style.yml +++ b/.rubocop/style.yml @@ -36,9 +36,11 @@ Style/OptionHash: Enabled: true Exclude: - 'lib/http/chainable.rb' + - 'lib/http/chainable/verbs.rb' - 'lib/http/client.rb' - 'lib/http/options.rb' - 'lib/http/redirector.rb' + - 'lib/http/session.rb' - 'lib/http/timeout/null.rb' - 'test/support/dummy_server.rb' diff --git a/CHANGELOG.md b/CHANGELOG.md index fd67bd41..264d5a9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING** Chainable option methods (`.headers`, `.timeout`, `.cookies`, + `.auth`, `.follow`, `.via`, `.use`, `.encoding`, `.nodelay`, `.basic_auth`, + `.accept`) now return a thread-safe `HTTP::Session` instead of `HTTP::Client`. + `Session` creates a new `Client` for each request, making it safe to share a + configured session across threads. `HTTP.persistent` still returns + `HTTP::Client` since persistent connections require mutable state. Code that + calls HTTP verb methods (`.get`, `.post`, etc.) or accesses `.default_options` + is unaffected. Code that checks `is_a?(HTTP::Client)` on the return value of + chainable methods will need to be updated to check for `HTTP::Session` +- **BREAKING** `.retriable` now returns `HTTP::Retriable::Session` instead of + `HTTP::Retriable::Client` - Improved error message when request body size cannot be determined to suggest setting `Content-Length` explicitly or using chunked `Transfer-Encoding` (#560) - **BREAKING** `Connection#readpartial` now raises `EOFError` instead of diff --git a/README.md b/README.md index 507d2773..0fe3249a 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,34 @@ end Pattern matching is also supported on `HTTP::Response::Status`, `HTTP::Headers`, `HTTP::ContentType`, and `HTTP::URI`. +### Thread Safety + +Configured sessions are safe to share across threads: + +```ruby +# Build a session once, use it from any thread +session = HTTP.headers("Accept" => "application/json") + .timeout(10) + .auth("Bearer token") + +threads = 10.times.map do + Thread.new { session.get("https://example.com/api/data") } +end +threads.each(&:join) +``` + +Chainable configuration methods (`.headers`, `.timeout`, `.auth`, etc.) return +an `HTTP::Session`, which creates a fresh `HTTP::Client` for every request. + +Persistent connections (`HTTP.persistent`) return an `HTTP::Client`, which is +**not** thread-safe. For thread-safe persistent connections, use the +[connection_pool](https://rubygems.org/gems/connection_pool) gem: + +```ruby +pool = ConnectionPool.new(size: 5) { HTTP.persistent("https://example.com") } +pool.with { |http| http.get("/path") } +``` + ## Supported Ruby Versions This library aims to support and is [tested against][build-link] diff --git a/lib/http.rb b/lib/http.rb index 416e2f8f..5583ddef 100644 --- a/lib/http.rb +++ b/lib/http.rb @@ -5,8 +5,10 @@ require "http/timeout/per_operation" require "http/timeout/global" require "http/chainable" +require "http/session" require "http/client" require "http/retriable/client" +require "http/retriable/session" require "http/connection" require "http/options" require "http/feature" @@ -21,14 +23,14 @@ module HTTP extend Chainable class << self - # Set default headers and return a chainable client instance + # Set default headers and return a chainable session # # @example # HTTP[:accept => "text/html"].get("https://example.com") # # @param headers [Hash] headers to set # - # @return [HTTP::Client] + # @return [HTTP::Session] # # @api public alias [] headers diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index f2005f04..b06cec4f 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -2,129 +2,14 @@ require "http/base64" require "http/chainable/helpers" +require "http/chainable/verbs" require "http/headers" module HTTP # HTTP verb methods and client configuration DSL module Chainable include HTTP::Base64 - - # Request a get sans response body - # - # @example - # HTTP.head("http://example.com") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def head(uri, options = {}) - request :head, uri, options - end - - # Get a resource - # - # @example - # HTTP.get("http://example.com") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def get(uri, options = {}) - request :get, uri, options - end - - # Post to a resource - # - # @example - # HTTP.post("http://example.com", body: "data") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def post(uri, options = {}) - request :post, uri, options - end - - # Put to a resource - # - # @example - # HTTP.put("http://example.com", body: "data") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def put(uri, options = {}) - request :put, uri, options - end - - # Delete a resource - # - # @example - # HTTP.delete("http://example.com/resource") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def delete(uri, options = {}) - request :delete, uri, options - end - - # Echo the request back to the client - # - # @example - # HTTP.trace("http://example.com") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def trace(uri, options = {}) - request :trace, uri, options - end - - # Return the methods supported on the given URI - # - # @example - # HTTP.options("http://example.com") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def options(uri, options = {}) - request :options, uri, options - end - - # Convert to a transparent TCP/IP tunnel - # - # @example - # HTTP.connect("http://example.com") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def connect(uri, options = {}) - request :connect, uri, options - end - - # Apply partial modifications to a resource - # - # @example - # HTTP.patch("http://example.com/resource", body: "data") - # - # @param [String, URI] uri URI to request - # @param [Hash] options request options - # @return [HTTP::Response] - # @api public - def patch(uri, options = {}) - request :patch, uri, options - end + include Verbs # Make an HTTP request with the given verb # @@ -135,7 +20,7 @@ def patch(uri, options = {}) # @return [HTTP::Response] # @api public def request(verb, uri, opts = {}) - branch(default_options).request(verb, uri, opts) + make_client(default_options).request(verb, uri, opts) end # Prepare an HTTP request with the given verb @@ -147,7 +32,7 @@ def request(verb, uri, opts = {}) # @return [HTTP::Request] # @api public def build_request(verb, uri, opts = {}) - branch(default_options).build_request(verb, uri, opts) + make_client(default_options).build_request(verb, uri, opts) end # Set timeout on the request @@ -165,7 +50,7 @@ def build_request(verb, uri, opts = {}) # @overload timeout(global_timeout) # Adds a global timeout to the full request # @param [Numeric] global_timeout - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def timeout(options) klass, options = case options @@ -222,7 +107,8 @@ def timeout(options) # @return [HTTP::Client, Object] # @api public def persistent(host, timeout: 5) - p_client = branch default_options.merge(keep_alive_timeout: timeout).with_persistent(host) + options = default_options.merge(keep_alive_timeout: timeout).with_persistent(host) + p_client = make_client(options) return p_client unless block_given? yield p_client @@ -237,7 +123,7 @@ def persistent(host, timeout: 5) # # @param [Array] proxy # @raise [Request::Error] if HTTP proxy is invalid - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def via(*proxy) proxy_hash = build_proxy_hash(proxy) @@ -254,7 +140,7 @@ def via(*proxy) # HTTP.follow.get("http://example.com") # # @param [Hash] options redirect options - # @return [HTTP::Client] + # @return [HTTP::Session] # @see Redirector#initialize # @api public def follow(options = {}) @@ -267,7 +153,7 @@ def follow(options = {}) # HTTP.headers("Accept" => "text/plain").get("http://example.com") # # @param [Hash] headers request headers - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def headers(headers) branch default_options.with_headers(headers) @@ -279,7 +165,7 @@ def headers(headers) # HTTP.cookies(session: "abc123").get("http://example.com") # # @param [Hash] cookies cookies to set - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def cookies(cookies) branch default_options.with_cookies(cookies) @@ -291,7 +177,7 @@ def cookies(cookies) # HTTP.encoding("UTF-8").get("http://example.com") # # @param [String, Encoding] encoding encoding to use - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def encoding(encoding) branch default_options.with_encoding(encoding) @@ -303,7 +189,7 @@ def encoding(encoding) # HTTP.accept("application/json").get("http://example.com") # # @param [String, Symbol] type MIME type to accept - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def accept(type) headers Headers::ACCEPT => MimeType.normalize(type) @@ -315,7 +201,7 @@ def accept(type) # HTTP.auth("Bearer token123").get("http://example.com") # # @param [#to_s] value Authorization header value - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def auth(value) headers Headers::AUTHORIZATION => value.to_s @@ -330,7 +216,7 @@ def auth(value) # @param [#fetch] opts # @option opts [#to_s] :user # @option opts [#to_s] :pass - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def basic_auth(opts) user = opts.fetch(:user) @@ -368,7 +254,7 @@ def default_options=(opts) # @example # HTTP.nodelay.get("http://example.com") # - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def nodelay branch default_options.with_nodelay(true) @@ -380,13 +266,13 @@ def nodelay # HTTP.use(:auto_inflate).get("http://example.com") # # @param [Array] features features to enable - # @return [HTTP::Client] + # @return [HTTP::Session] # @api public def use(*features) branch default_options.with_features(features) end - # Return a retriable client that retries on failure + # Return a retriable session that retries on failure # # @example Usage # @@ -403,20 +289,29 @@ def use(*features) # HTTP.retriable(tries: 3, delay: proc { |i| 1 + i*i }).get(url) # # @param (see Performer#initialize) - # @return [HTTP::Retriable::Client] + # @return [HTTP::Retriable::Session] # @api public def retriable(**options) - Retriable::Client.new(Retriable::Performer.new(options), default_options) + Retriable::Session.new(Retriable::Performer.new(options), default_options) end private - # Create a new client with the given options + # Create a new session with the given options + # + # @param [HTTP::Options] options options for the session + # @return [HTTP::Session] + # @api private + def branch(options) + HTTP::Session.new(options) + end + + # Create a new client for executing a request # # @param [HTTP::Options] options options for the client # @return [HTTP::Client] # @api private - def branch(options) + def make_client(options) HTTP::Client.new(options) end end diff --git a/lib/http/chainable/verbs.rb b/lib/http/chainable/verbs.rb new file mode 100644 index 00000000..deed06a4 --- /dev/null +++ b/lib/http/chainable/verbs.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +module HTTP + module Chainable + # HTTP verb shortcut methods + # + # Each method delegates to {Chainable#request} with the appropriate verb. + module Verbs + # Request a get sans response body + # + # @example + # HTTP.head("http://example.com") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def head(uri, options = {}) + request :head, uri, options + end + + # Get a resource + # + # @example + # HTTP.get("http://example.com") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def get(uri, options = {}) + request :get, uri, options + end + + # Post to a resource + # + # @example + # HTTP.post("http://example.com", body: "data") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def post(uri, options = {}) + request :post, uri, options + end + + # Put to a resource + # + # @example + # HTTP.put("http://example.com", body: "data") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def put(uri, options = {}) + request :put, uri, options + end + + # Delete a resource + # + # @example + # HTTP.delete("http://example.com/resource") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def delete(uri, options = {}) + request :delete, uri, options + end + + # Echo the request back to the client + # + # @example + # HTTP.trace("http://example.com") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def trace(uri, options = {}) + request :trace, uri, options + end + + # Return the methods supported on the given URI + # + # @example + # HTTP.options("http://example.com") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def options(uri, options = {}) + request :options, uri, options + end + + # Convert to a transparent TCP/IP tunnel + # + # @example + # HTTP.connect("http://example.com") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def connect(uri, options = {}) + request :connect, uri, options + end + + # Apply partial modifications to a resource + # + # @example + # HTTP.patch("http://example.com/resource", body: "data") + # + # @param [String, URI] uri URI to request + # @param [Hash] options request options + # @return [HTTP::Response] + # @api public + def patch(uri, options = {}) + request :patch, uri, options + end + end + end +end diff --git a/lib/http/retriable/client.rb b/lib/http/retriable/client.rb index eb09a127..70182edd 100644 --- a/lib/http/retriable/client.rb +++ b/lib/http/retriable/client.rb @@ -39,12 +39,21 @@ def perform(req, options) private - # Creates a new branch of the retriable client + # Creates a new retriable session when chaining options # # @param [HTTP::Options] options # @api private - # @return [HTTP::Retriable::Client] + # @return [HTTP::Retriable::Session] def branch(options) + Retriable::Session.new(@performer, options) + end + + # Creates a new Retriable::Client for executing a single request + # + # @param [HTTP::Options] options + # @api private + # @return [HTTP::Retriable::Client] + def make_client(options) Retriable::Client.new(@performer, options) end end diff --git a/lib/http/retriable/session.rb b/lib/http/retriable/session.rb new file mode 100644 index 00000000..9fcd6d16 --- /dev/null +++ b/lib/http/retriable/session.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require "http/retriable/performer" + +module HTTP + module Retriable + # Thread-safe options builder with retry support. + # + # Returned by {Chainable#retriable}, this session creates a new + # {Retriable::Client} for each request, preserving retry behavior + # while remaining thread-safe. + # + # @see HTTP::Session + # @see Retriable::Client + class Session < HTTP::Session + # Initializes a retriable session + # + # @example + # HTTP::Retriable::Session.new(performer, options) + # + # @param [Performer] performer + # @param [HTTP::Options, Hash] options + # @api public + # @return [HTTP::Retriable::Session] + def initialize(performer, options) + @performer = performer + super(options) + end + + private + + # Creates a new branch of the retriable session + # + # @param [HTTP::Options] options + # @api private + # @return [HTTP::Retriable::Session] + def branch(options) + Retriable::Session.new(@performer, options) + end + + # Creates a Retriable::Client for executing a single request + # + # @param [HTTP::Options] options + # @api private + # @return [HTTP::Retriable::Client] + def make_client(options) + Retriable::Client.new(@performer, options) + end + end + end +end diff --git a/lib/http/session.rb b/lib/http/session.rb new file mode 100644 index 00000000..ee964458 --- /dev/null +++ b/lib/http/session.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require "forwardable" + +module HTTP + # Thread-safe options builder for configuring HTTP requests. + # + # Session objects are returned by all chainable configuration methods + # (e.g., {Chainable#headers}, {Chainable#timeout}, {Chainable#cookies}). + # They hold an immutable {Options} object and create a new {Client} + # for each request, making them safe to share across threads. + # + # @example Reuse a configured session across threads + # session = HTTP.headers("Accept" => "application/json").timeout(10) + # threads = 5.times.map do + # Thread.new { session.get("https://example.com") } + # end + # threads.each(&:join) + # + # @see Chainable + # @see Client + class Session + extend Forwardable + include Chainable + + # @!method persistent? + # Indicate whether the session has persistent connection options + # + # @example + # session = HTTP::Session.new(persistent: "http://example.com") + # session.persistent? + # + # @see Options#persistent? + # @return [Boolean] + # @api public + def_delegator :default_options, :persistent? + + # Initialize a new Session + # + # @example + # session = HTTP::Session.new(headers: {"Accept" => "application/json"}) + # + # @param default_options [Hash, Options] options for requests + # @return [HTTP::Session] a new session instance + # @api public + def initialize(default_options = {}) + @default_options = HTTP::Options.new(default_options) + end + + # Make an HTTP request by creating a new {Client} + # + # A fresh {Client} is created for each request, ensuring thread safety. + # + # @example + # session = HTTP::Session.new + # session.request(:get, "https://example.com") + # + # @param verb [Symbol] the HTTP method + # @param uri [#to_s] the URI to request + # @param opts [Hash] request options + # @return [HTTP::Response] the response + # @api public + def request(verb, uri, opts = {}) + make_client(default_options).request(verb, uri, opts) + end + + # Build an HTTP request without executing it + # + # @example + # session = HTTP::Session.new + # session.build_request(:get, "https://example.com") + # + # @param verb [Symbol] the HTTP method + # @param uri [#to_s] the URI to request + # @param opts [Hash] request options + # @return [HTTP::Request] the built request object + # @api public + def build_request(verb, uri, opts = {}) + make_client(default_options).build_request(verb, uri, opts) + end + end +end diff --git a/sig/http.rbs b/sig/http.rbs index 2d444129..48d85ecb 100644 --- a/sig/http.rbs +++ b/sig/http.rbs @@ -5,7 +5,7 @@ module HTTP extend Chainable - def self.[]: (untyped headers) -> Client + def self.[]: (untyped headers) -> Session module Base64 def self?.encode64: (String input) -> String @@ -13,42 +13,51 @@ module HTTP module Chainable include Base64 + include Chainable::Verbs PROXY_ARG_MAP: Array[Array[untyped]] @default_options: Options - def head: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def get: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def post: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def put: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def delete: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def trace: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def options: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def connect: (untyped uri, ?::Hash[untyped, untyped] options) -> Response - def patch: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + module Verbs : _VerbsHost + interface _VerbsHost + def request: (Symbol verb, untyped uri, ?untyped opts) -> Response + end + + def head: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def get: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def post: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def put: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def delete: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def trace: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def options: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def connect: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + def patch: (untyped uri, ?::Hash[untyped, untyped] options) -> Response + end + def request: (Symbol verb, untyped uri, ?untyped opts) -> Response def build_request: (Symbol verb, untyped uri, ?untyped opts) -> Request - def timeout: (untyped options) -> Client + def timeout: (untyped options) -> Session def persistent: (String host, ?timeout: Integer) ?{ (Client) -> untyped } -> untyped - def via: (*untyped proxy) -> Client + def via: (*untyped proxy) -> Session alias through via - def follow: (?::Hash[untyped, untyped] options) -> Client - def headers: (untyped headers) -> Client - def cookies: (untyped cookies) -> Client - def encoding: (untyped encoding) -> Client - def accept: (untyped type) -> Client - def auth: (untyped value) -> Client - def basic_auth: (untyped opts) -> Client + def follow: (?::Hash[untyped, untyped] options) -> Session + def headers: (untyped headers) -> Session + def cookies: (untyped cookies) -> Session + def encoding: (untyped encoding) -> Session + def accept: (untyped type) -> Session + def auth: (untyped value) -> Session + def basic_auth: (untyped opts) -> Session def default_options: () -> Options def default_options=: (untyped opts) -> Options - def nodelay: () -> Client - def use: (*untyped features) -> Client - def retriable: (**untyped options) -> Retriable::Client + def nodelay: () -> Session + def use: (*untyped features) -> Session + def retriable: (**untyped options) -> Retriable::Session private - def branch: (Options options) -> Client + def branch: (Options options) -> Session + def make_client: (Options options) -> Client def build_proxy_hash: (Array[untyped] proxy) -> Hash[Symbol, untyped] def resolve_timeout_hash: (Hash[Symbol, untyped] options) -> [Class, Hash[Symbol, untyped]] def resolve_global_only: (Numeric? global) -> [Class, Hash[Symbol, untyped]] @@ -96,6 +105,18 @@ module HTTP end end + class Session + extend Forwardable + include Chainable + + @default_options: Options + + def initialize: (?untyped default_options) -> void + def request: (Symbol verb, untyped uri, ?untyped opts) -> Response + def build_request: (Symbol verb, untyped uri, ?untyped opts) -> Request + def persistent?: () -> bool + end + class Headers extend Forwardable include Enumerable[Array[String]] @@ -1038,6 +1059,17 @@ module HTTP end module Retriable + class Session < HTTP::Session + @performer: untyped + + def initialize: (untyped performer, untyped options) -> void + + private + + def branch: (Options options) -> Retriable::Session + def make_client: (Options options) -> Retriable::Client + end + class Client < HTTP::Client @performer: untyped @@ -1046,7 +1078,8 @@ module HTTP private - def branch: (Options options) -> Retriable::Client + def branch: (Options options) -> Retriable::Session + def make_client: (Options options) -> Retriable::Client end class Performer diff --git a/test/http/retriable/client_test.rb b/test/http/retriable/client_test.rb index 5fd55bd0..61eefeba 100644 --- a/test/http/retriable/client_test.rb +++ b/test/http/retriable/client_test.rb @@ -2,16 +2,30 @@ require "test_helper" +require "support/dummy_server" + describe HTTP::Retriable::Client do cover "HTTP::Retriable::Client*" - describe "#branch (private)" do - let(:performer) { HTTP::Retriable::Performer.new({}) } - let(:client) { HTTP::Retriable::Client.new(performer, HTTP::Options.new) } + run_server(:dummy) { DummyServer.new } - it "returns a Retriable::Client when chaining" do + let(:performer) { HTTP::Retriable::Performer.new({}) } + let(:client) { HTTP::Retriable::Client.new(performer, HTTP::Options.new) } + + describe "#branch (private)" do + it "returns a Retriable::Session when chaining" do chained = client.headers("Accept" => "text/html") - assert_kind_of HTTP::Retriable::Client, chained + assert_kind_of HTTP::Retriable::Session, chained + end + end + + describe "#make_client (private)" do + it "creates a Retriable::Client for persistent connections" do + p_client = client.persistent(dummy.endpoint) + + assert_kind_of HTTP::Retriable::Client, p_client + ensure + p_client&.close end end end diff --git a/test/http/retriable/session_test.rb b/test/http/retriable/session_test.rb new file mode 100644 index 00000000..b95de476 --- /dev/null +++ b/test/http/retriable/session_test.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require "test_helper" + +require "support/dummy_server" + +describe HTTP::Retriable::Session do + cover "HTTP::Retriable::Session*" + run_server(:dummy) { DummyServer.new } + + let(:performer) { HTTP::Retriable::Performer.new({}) } + let(:session) { HTTP::Retriable::Session.new(performer, HTTP::Options.new) } + + describe "#branch (private)" do + it "returns a Retriable::Session when chaining" do + chained = session.headers("Accept" => "text/html") + + assert_kind_of HTTP::Retriable::Session, chained + end + + it "preserves performer through chaining" do + chained = session.headers("Accept" => "text/html") + .timeout(10) + + assert_kind_of HTTP::Retriable::Session, chained + end + end + + describe "#make_client (private)" do + it "creates a Retriable::Client for persistent connections" do + p_client = session.persistent(dummy.endpoint) + + assert_kind_of HTTP::Retriable::Client, p_client + ensure + p_client&.close + end + end +end diff --git a/test/http/session_test.rb b/test/http/session_test.rb new file mode 100644 index 00000000..900cf9f8 --- /dev/null +++ b/test/http/session_test.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +require "test_helper" + +require "support/dummy_server" + +describe HTTP::Session do + cover "HTTP::Session*" + run_server(:dummy) { DummyServer.new } + + let(:session) { HTTP::Session.new } + + describe "#initialize" do + it "creates a session with default options" do + assert_kind_of HTTP::Options, session.default_options + end + + it "creates a session with given options" do + session = HTTP::Session.new(headers: { "Accept" => "text/html" }) + + assert_equal "text/html", session.default_options.headers[:accept] + end + end + + describe "#request" do + it "returns an HTTP::Response" do + response = session.request(:get, dummy.endpoint) + + assert_kind_of HTTP::Response, response + end + + it "creates a new client for each request" do + client_ids = [] + original_new = HTTP::Client.method(:new) + + HTTP::Client.stub(:new, lambda { |*args| + c = original_new.call(*args) + client_ids << c.object_id + c + }) do + session.get(dummy.endpoint) + session.get(dummy.endpoint) + end + + assert_equal 2, client_ids.uniq.size + end + end + + describe "#build_request" do + it "returns an HTTP::Request" do + req = session.build_request(:get, "http://example.com/") + + assert_kind_of HTTP::Request, req + end + end + + describe "#persistent?" do + it "returns false by default" do + refute_predicate session, :persistent? + end + end + + describe "chaining" do + it "returns a Session from headers" do + chained = session.headers("Accept" => "text/html") + + assert_kind_of HTTP::Session, chained + end + + it "returns a Session from timeout" do + chained = session.timeout(10) + + assert_kind_of HTTP::Session, chained + end + + it "returns a Session from cookies" do + chained = session.cookies(session_id: "abc") + + assert_kind_of HTTP::Session, chained + end + + it "returns a Session from follow" do + chained = session.follow + + assert_kind_of HTTP::Session, chained + end + + it "returns a Session from use" do + chained = session.use(:auto_deflate) + + assert_kind_of HTTP::Session, chained + end + + it "returns a Session from nodelay" do + chained = session.nodelay + + assert_kind_of HTTP::Session, chained + end + + it "returns a Session from encoding" do + chained = session.encoding("UTF-8") + + assert_kind_of HTTP::Session, chained + end + + it "returns a Session from via" do + chained = session.via("proxy.example.com", 8080) + + assert_kind_of HTTP::Session, chained + end + + it "preserves options through chaining" do + chained = session.headers("Accept" => "text/html") + .timeout(10) + .cookies(session_id: "abc") + + assert_equal "text/html", chained.default_options.headers[:accept] + assert_equal HTTP::Timeout::Global, chained.default_options.timeout_class + assert_equal "session_id=abc", chained.default_options.cookies["session_id"] + end + end + + describe "thread safety" do + it "can be shared across threads without errors" do + shared_session = HTTP.headers("Accept" => "text/html").timeout(5) + errors = [] + mutex = Mutex.new + + threads = Array.new(5) do + Thread.new do + shared_session.get(dummy.endpoint) + rescue => e + mutex.synchronize { errors << e } + end + end + threads.each(&:join) + + assert_empty errors, "Expected no errors but got: #{errors.map(&:message).join(', ')}" + end + end + + describe "persistent" do + it "returns an HTTP::Client" do + p_client = HTTP::Session.new.persistent(dummy.endpoint) + + assert_kind_of HTTP::Client, p_client + ensure + p_client&.close + end + end +end diff --git a/test/http_test.rb b/test/http_test.rb index 592d6d1e..b1ae55f3 100644 --- a/test/http_test.rb +++ b/test/http_test.rb @@ -754,10 +754,10 @@ def setsockopt(*args) end describe ".encoding" do - it "returns a client with the specified encoding" do - client = HTTP::Client.new.encoding("UTF-8") + it "returns a session with the specified encoding" do + session = HTTP::Client.new.encoding("UTF-8") - assert_kind_of HTTP::Client, client + assert_kind_of HTTP::Session, session end end From b4fe926157fbcba569dcabdde18539800fd88603 Mon Sep 17 00:00:00 2001 From: Erik Berlin Date: Mon, 9 Mar 2026 03:31:23 -0700 Subject: [PATCH 2/2] Refactor retriable from separate class hierarchy to session option Make retriable a first-class option on HTTP::Options (like `follow`), eliminating the need for Retriable::Client and Retriable::Session. Client#perform now checks options.retriable and delegates to Retriable::Performer when set, keeping retry as a per-request concern that flows naturally through the options system. --- CHANGELOG.md | 6 ++- lib/http.rb | 2 - lib/http/chainable.rb | 4 +- lib/http/client.rb | 51 ++++++++++++++++++------ lib/http/options/definitions.rb | 16 ++++++++ lib/http/retriable/client.rb | 61 ----------------------------- lib/http/retriable/session.rb | 51 ------------------------ sig/http.rbs | 30 +++----------- test/http/options/merge_test.rb | 1 + test/http/options_test.rb | 7 ++++ test/http/retriable/client_test.rb | 31 --------------- test/http/retriable/session_test.rb | 38 ------------------ test/http/session_test.rb | 6 +++ 13 files changed, 81 insertions(+), 223 deletions(-) delete mode 100644 lib/http/retriable/client.rb delete mode 100644 lib/http/retriable/session.rb delete mode 100644 test/http/retriable/client_test.rb delete mode 100644 test/http/retriable/session_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 264d5a9b..de550ec3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,8 +40,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 calls HTTP verb methods (`.get`, `.post`, etc.) or accesses `.default_options` is unaffected. Code that checks `is_a?(HTTP::Client)` on the return value of chainable methods will need to be updated to check for `HTTP::Session` -- **BREAKING** `.retriable` now returns `HTTP::Retriable::Session` instead of - `HTTP::Retriable::Client` +- **BREAKING** `.retriable` now returns `HTTP::Session` instead of + `HTTP::Retriable::Client`. Retry is a session-level option: it flows through + `HTTP::Options` into `HTTP::Client#perform`, eliminating the need for + separate `Retriable::Client` and `Retriable::Session` classes - Improved error message when request body size cannot be determined to suggest setting `Content-Length` explicitly or using chunked `Transfer-Encoding` (#560) - **BREAKING** `Connection#readpartial` now raises `EOFError` instead of diff --git a/lib/http.rb b/lib/http.rb index 5583ddef..63e91ddf 100644 --- a/lib/http.rb +++ b/lib/http.rb @@ -7,8 +7,6 @@ require "http/chainable" require "http/session" require "http/client" -require "http/retriable/client" -require "http/retriable/session" require "http/connection" require "http/options" require "http/feature" diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index b06cec4f..bd391bcd 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -289,10 +289,10 @@ def use(*features) # HTTP.retriable(tries: 3, delay: proc { |i| 1 + i*i }).get(url) # # @param (see Performer#initialize) - # @return [HTTP::Retriable::Session] + # @return [HTTP::Session] # @api public def retriable(**options) - Retriable::Session.new(Retriable::Performer.new(options), default_options) + branch default_options.with_retriable(options.empty? || options) end private diff --git a/lib/http/client.rb b/lib/http/client.rb index 81bb7390..1b488a20 100644 --- a/lib/http/client.rb +++ b/lib/http/client.rb @@ -4,6 +4,7 @@ require "http/client/request_builder" require "http/form_data" +require "http/retriable/performer" require "http/options" require "http/feature" require "http/headers" @@ -105,6 +106,35 @@ def build_request(verb, uri, opts = {}) # @return [HTTP::Response] the response # @api public def perform(req, options) + if options.retriable + perform_with_retry(req, options) + else + perform_once(req, options) + end + end + + # Close the connection and reset state + # + # @example + # client.close + # + # @return [void] + # @api public + def close + @connection&.close + @connection = nil + @state = :clean + end + + private + + # Execute a single HTTP request without retry logic + # + # @param req [HTTP::Request] the request to perform + # @param options [HTTP::Options] request options + # @return [HTTP::Response] the response + # @api private + def perform_once(req, options) verify_connection!(req.uri) @state = :dirty @@ -121,21 +151,18 @@ def perform(req, options) raise end - # Close the connection and reset state - # - # @example - # client.close + # Execute a request with retry logic # - # @return [void] - # @api public - def close - @connection&.close - @connection = nil - @state = :clean + # @param req [HTTP::Request] the request to perform + # @param options [HTTP::Options] request options + # @return [HTTP::Response] the response + # @api private + def perform_with_retry(req, options) + Retriable::Performer.new(options.retriable).perform(self, req) do + perform_once(req, options) + end end - private - # Send request over the connection, handling proxy and errors # @return [void] # @api private diff --git a/lib/http/options/definitions.rb b/lib/http/options/definitions.rb index 106f41d9..cb75b8fd 100644 --- a/lib/http/options/definitions.rb +++ b/lib/http/options/definitions.rb @@ -81,6 +81,22 @@ def follow=(value) end end + def_option :retriable, reader_only: true + + # Sets retriable options + # + # @param [Boolean, Hash, nil] value + # @api private + # @return [Hash, nil] + def retriable=(value) + @retriable = + if !value then nil + elsif true == value then Hash[] + elsif value.respond_to?(:fetch) then value + else argument_error! "Unsupported retriable options: #{value}" + end + end + def_option :persistent, reader_only: true # Sets persistent connection origin diff --git a/lib/http/retriable/client.rb b/lib/http/retriable/client.rb deleted file mode 100644 index 70182edd..00000000 --- a/lib/http/retriable/client.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require "http/retriable/performer" - -module HTTP - # Namespace for retriable HTTP client functionality - module Retriable - # Retriable version of HTTP::Client. - # - # @see http://www.rubydoc.info/gems/http/HTTP/Client - class Client < HTTP::Client - # Initializes a retriable client - # - # @example - # HTTP::Retriable::Client.new(performer, options) - # - # @param [Performer] performer - # @param [HTTP::Options, Hash] options - # @api public - # @return [HTTP::Retriable::Client] - def initialize(performer, options) - @performer = performer - super(options) - end - - # Performs request with retry logic - # - # @example - # client.perform(request, options) - # - # @param [HTTP::Request] req - # @param [HTTP::Options] options - # @see http://www.rubydoc.info/gems/http/HTTP/Client:perform - # @api public - # @return [HTTP::Response] - def perform(req, options) - @performer.perform(self, req) { super(req, options) } - end - - private - - # Creates a new retriable session when chaining options - # - # @param [HTTP::Options] options - # @api private - # @return [HTTP::Retriable::Session] - def branch(options) - Retriable::Session.new(@performer, options) - end - - # Creates a new Retriable::Client for executing a single request - # - # @param [HTTP::Options] options - # @api private - # @return [HTTP::Retriable::Client] - def make_client(options) - Retriable::Client.new(@performer, options) - end - end - end -end diff --git a/lib/http/retriable/session.rb b/lib/http/retriable/session.rb deleted file mode 100644 index 9fcd6d16..00000000 --- a/lib/http/retriable/session.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -require "http/retriable/performer" - -module HTTP - module Retriable - # Thread-safe options builder with retry support. - # - # Returned by {Chainable#retriable}, this session creates a new - # {Retriable::Client} for each request, preserving retry behavior - # while remaining thread-safe. - # - # @see HTTP::Session - # @see Retriable::Client - class Session < HTTP::Session - # Initializes a retriable session - # - # @example - # HTTP::Retriable::Session.new(performer, options) - # - # @param [Performer] performer - # @param [HTTP::Options, Hash] options - # @api public - # @return [HTTP::Retriable::Session] - def initialize(performer, options) - @performer = performer - super(options) - end - - private - - # Creates a new branch of the retriable session - # - # @param [HTTP::Options] options - # @api private - # @return [HTTP::Retriable::Session] - def branch(options) - Retriable::Session.new(@performer, options) - end - - # Creates a Retriable::Client for executing a single request - # - # @param [HTTP::Options] options - # @api private - # @return [HTTP::Retriable::Client] - def make_client(options) - Retriable::Client.new(@performer, options) - end - end - end -end diff --git a/sig/http.rbs b/sig/http.rbs index 48d85ecb..f63b1e33 100644 --- a/sig/http.rbs +++ b/sig/http.rbs @@ -52,7 +52,7 @@ module HTTP def default_options=: (untyped opts) -> Options def nodelay: () -> Session def use: (*untyped features) -> Session - def retriable: (**untyped options) -> Retriable::Session + def retriable: (**untyped options) -> Session private @@ -83,6 +83,8 @@ module HTTP private + def perform_once: (Request req, Options options) -> Response + def perform_with_retry: (Request req, Options options) -> Response def wrap_request: (Request req, Options opts) -> Request def build_response: (Request req, Options options) -> Response def build_wrapped_response: (Request req, Options options) -> Response @@ -428,6 +430,7 @@ module HTTP attr_reader features: Hash[Symbol, Feature] def follow=: (untyped value) -> untyped + def retriable=: (untyped value) -> untyped def persistent=: (untyped value) -> String? def persistent?: () -> bool def merge: (untyped other) -> Options @@ -454,6 +457,7 @@ module HTTP attr_accessor cookies: Hash[untyped, untyped] attr_accessor encoding: Encoding? attr_reader follow: Hash[untyped, untyped]? + attr_reader retriable: Hash[untyped, untyped]? attr_reader persistent: String? def with_headers: (untyped value) -> Options @@ -461,6 +465,7 @@ module HTTP def with_encoding: (untyped value) -> Options def with_features: (untyped value) -> Options def with_follow: (untyped value) -> Options + def with_retriable: (untyped value) -> Options def with_persistent: (untyped value) -> Options def with_proxy: (untyped value) -> Options def with_params: (untyped value) -> Options @@ -1059,29 +1064,6 @@ module HTTP end module Retriable - class Session < HTTP::Session - @performer: untyped - - def initialize: (untyped performer, untyped options) -> void - - private - - def branch: (Options options) -> Retriable::Session - def make_client: (Options options) -> Retriable::Client - end - - class Client < HTTP::Client - @performer: untyped - - def initialize: (untyped performer, untyped options) -> void - def perform: (Request req, Options options) -> Response - - private - - def branch: (Options options) -> Retriable::Session - def make_client: (Options options) -> Retriable::Client - end - class Performer RETRIABLE_ERRORS: Array[untyped] diff --git a/test/http/options/merge_test.rb b/test/http/options/merge_test.rb index 8b32b282..55d2b701 100644 --- a/test/http/options/merge_test.rb +++ b/test/http/options/merge_test.rb @@ -62,6 +62,7 @@ headers: { "Foo" => "foo", "Accept" => "xml", "Bar" => "bar" }, proxy: { proxy_address: "127.0.0.1", proxy_port: 8080 }, follow: nil, + retriable: nil, socket_class: HTTP::Options.default_socket_class, nodelay: false, ssl_socket_class: HTTP::Options.default_ssl_socket_class, diff --git a/test/http/options_test.rb b/test/http/options_test.rb index 3299fc26..0f5c49df 100644 --- a/test/http/options_test.rb +++ b/test/http/options_test.rb @@ -29,6 +29,13 @@ end end + describe "#with_retriable" do + it "raises error for unsupported retriable options" do + err = assert_raises(HTTP::Error) { subject_under_test.with_retriable(42) } + assert_match(/Unsupported retriable/, err.message) + end + end + describe "#dup" do it "returns a duplicate without a block" do dupped = subject_under_test.dup diff --git a/test/http/retriable/client_test.rb b/test/http/retriable/client_test.rb deleted file mode 100644 index 61eefeba..00000000 --- a/test/http/retriable/client_test.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -require "support/dummy_server" - -describe HTTP::Retriable::Client do - cover "HTTP::Retriable::Client*" - run_server(:dummy) { DummyServer.new } - - let(:performer) { HTTP::Retriable::Performer.new({}) } - let(:client) { HTTP::Retriable::Client.new(performer, HTTP::Options.new) } - - describe "#branch (private)" do - it "returns a Retriable::Session when chaining" do - chained = client.headers("Accept" => "text/html") - - assert_kind_of HTTP::Retriable::Session, chained - end - end - - describe "#make_client (private)" do - it "creates a Retriable::Client for persistent connections" do - p_client = client.persistent(dummy.endpoint) - - assert_kind_of HTTP::Retriable::Client, p_client - ensure - p_client&.close - end - end -end diff --git a/test/http/retriable/session_test.rb b/test/http/retriable/session_test.rb deleted file mode 100644 index b95de476..00000000 --- a/test/http/retriable/session_test.rb +++ /dev/null @@ -1,38 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -require "support/dummy_server" - -describe HTTP::Retriable::Session do - cover "HTTP::Retriable::Session*" - run_server(:dummy) { DummyServer.new } - - let(:performer) { HTTP::Retriable::Performer.new({}) } - let(:session) { HTTP::Retriable::Session.new(performer, HTTP::Options.new) } - - describe "#branch (private)" do - it "returns a Retriable::Session when chaining" do - chained = session.headers("Accept" => "text/html") - - assert_kind_of HTTP::Retriable::Session, chained - end - - it "preserves performer through chaining" do - chained = session.headers("Accept" => "text/html") - .timeout(10) - - assert_kind_of HTTP::Retriable::Session, chained - end - end - - describe "#make_client (private)" do - it "creates a Retriable::Client for persistent connections" do - p_client = session.persistent(dummy.endpoint) - - assert_kind_of HTTP::Retriable::Client, p_client - ensure - p_client&.close - end - end -end diff --git a/test/http/session_test.rb b/test/http/session_test.rb index 900cf9f8..d5c0ffd2 100644 --- a/test/http/session_test.rb +++ b/test/http/session_test.rb @@ -109,6 +109,12 @@ assert_kind_of HTTP::Session, chained end + it "returns a Session from retriable" do + chained = session.retriable + + assert_kind_of HTTP::Session, chained + end + it "preserves options through chaining" do chained = session.headers("Accept" => "text/html") .timeout(10)