Per-request read timeout on KB::Client#request (1.1.0)

CHANGELOG.md

@@ -6,7 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [unreleased]
-- See diff: https://github.com/barkibu/kb-ruby/compare/v1.0.0...HEAD
+- See diff: https://github.com/barkibu/kb-ruby/compare/v1.1.0...HEAD
+
+## [1.1.0]
+- Add `read_timeout:` to `KB::Client#request` to raise the read budget for a single call (e.g. `GET /v1/pets/birthdays`, whose server-side work runs for seconds). Connect and write budgets stay global; the override does not leak into later calls on the same connection.
 
 ## [1.0.0]
 - [Breaking changes] Split the single global request timeout into per-phase budgets: `KB.config.request.connect_timeout` (default 1s, bounds TCP connect + TLS handshake), `write_timeout` (default 3s), `read_timeout` (default 5s). `KB.config.request.timeout` is removed — assigning it now raises `NoMethodError` at boot. Migration: a previous global `timeout` maps to `read_timeout` (e.g. `KB_REQUEST_TIMEOUT_SECONDS=12` → `read_timeout = 12`).

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    barkibu-kb (1.0.0)
+    barkibu-kb (1.1.0)
       activemodel (>= 4.0.2)
       activerecord
       activesupport (>= 3.0.0)
@@ -10,8 +10,8 @@ PATH
       faraday-net_http (~> 1.0)
       faraday_middleware
       i18n
-    barkibu-kb-fake (1.0.0)
-      barkibu-kb (= 1.0.0)
+    barkibu-kb-fake (1.1.0)
+      barkibu-kb (= 1.1.0)
       countries
       sinatra
       webmock

README.md

@@ -66,6 +66,14 @@ KB.config.request.write_timeout = 4   # 3 by default
 KB.config.request.read_timeout = 10   # 5 by default
 ```
 
+The read budget can be raised for a single call through `KB::Client#request`, for
+the few endpoints whose server-side work legitimately runs for seconds. Connect
+and write budgets stay global:
+
+```ruby
+KB::Pet.kb_client.request('birthdays', filters: { month: 9, day: 22, size: 1000 }, read_timeout: 30)
+```
+
 ### Exposed Entities
 
 #### Pet Parent 🧍🏾

lib/kb/client.rb

@@ -7,12 +7,16 @@ def initialize(base_url, api_key: ENV['KB_API_KEY'])
       @base_url = base_url
     end
 
-    def request(sub_path, filters: nil, method: :get)
-      return connection.public_send(method, sub_path, attributes_to_json(filters)).body if method != :get
+    # `read_timeout` overrides KB.config.request.read_timeout for this one call only,
+    # for the few endpoints whose server-side work legitimately runs for seconds
+    # (e.g. GET /v1/pets/birthdays). Connect and write budgets stay global.
+    def request(sub_path, filters: nil, method: :get, read_timeout: nil)
+      options = request_options(read_timeout)
+      return connection.public_send(method, sub_path, attributes_to_json(filters), &options).body if method != :get
 
       cache_key = "#{@base_url}/#{sub_path}/#{(filters || {}).sort.to_h}"
       KB::Cache.fetch(cache_key) do
-        connection.public_send(method, sub_path, filters).body
+        connection.public_send(method, sub_path, filters, &options).body
       end
     end
 
@@ -86,6 +90,12 @@ def connection
       end
     end
 
+    def request_options(read_timeout)
+      return nil if read_timeout.nil?
+
+      ->(req) { req.options.read_timeout = read_timeout }
+    end
+
     def request_timeouts
       {
         open_timeout: KB.config.request.connect_timeout,

lib/kb/version.rb

@@ -1,3 +1,3 @@
 module KB
-  VERSION = '1.0.0'.freeze
+  VERSION = '1.1.0'.freeze
 end

spec/client_read_timeout_socket_spec.rb

@@ -0,0 +1,52 @@
+require 'spec_helper'
+require 'socket'
+
+# End-to-end check that a per-request read timeout reaches the socket. A local
+# TCP server accepts the connection, reads the request and never answers, so
+# the only thing that can end the call is Net::HTTP's read timeout.
+RSpec.describe KB::Client, '#request' do
+  let(:server) { TCPServer.new('127.0.0.1', 0) }
+  let(:port) { server.addr[1] }
+  let(:client) { described_class.new("http://127.0.0.1:#{port}/v1/pets", api_key: 'test') }
+
+  let!(:stalled_server) do
+    Thread.new do
+      loop do
+        socket = server.accept
+        socket.readpartial(4096) # consume the request, then stay silent
+      rescue IOError, Errno::EBADF
+        break
+      end
+    end
+  end
+
+  around do |example|
+    WebMock.allow_net_connect!(net_http_connect_on_start: false)
+    example.run
+  ensure
+    WebMock.disable_net_connect!
+  end
+
+  after do
+    server.close
+    stalled_server.join(1)
+  end
+
+  def elapsed
+    started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+    yield
+    Process.clock_gettime(Process::CLOCK_MONOTONIC) - started
+  end
+
+  it 'gives up after the per-request read timeout, not the 5s global default' do
+    error = nil
+    seconds = elapsed do
+      client.request('birthdays', filters: { month: 9 }, read_timeout: 1)
+    rescue Faraday::Error => e
+      error = e
+    end
+
+    expect(error: error.class, within_budget: seconds.between?(0.9, 2.5))
+      .to eq(error: Faraday::TimeoutError, within_budget: true)
+  end
+end

spec/client_spec.rb

@@ -208,5 +208,56 @@
       end
     end
   end
+
+  describe '#request' do
+    let(:api_response) { [200, { 'Content-Type': 'application/json' }, { elements: [] }.to_json] }
+    let(:sub_path) { 'birthdays' }
+    let(:filters) { { month: 9, day: 22 } }
+
+    it 'launches a GET request on the sub path with filters as params' do
+      stubs.get("#{path}/#{sub_path}") do |env|
+        expect(env.params).to include filters.transform_keys(&:to_s).transform_values(&:to_s)
+        api_response
+      end
+      client.request(sub_path, filters: filters)
+      stubs.verify_stubbed_calls
+    end
+
+    it 'uses the global read timeout by default' do
+      stubs.get("#{path}/#{sub_path}") do |env|
+        expect(env.request.read_timeout).to eq KB.config.request.read_timeout
+        api_response
+      end
+      client.request(sub_path, filters: filters)
+    end
+
+    it 'overrides only the read timeout for that call when given' do
+      stubs.get("#{path}/#{sub_path}") do |env|
+        expect(
+          open: env.request.open_timeout, write: env.request.write_timeout, read: env.request.read_timeout
+        ).to eq(open: KB.config.request.connect_timeout, write: KB.config.request.write_timeout, read: 30)
+        api_response
+      end
+      client.request(sub_path, filters: filters, read_timeout: 30)
+    end
+
+    it 'does not leak the override into later calls' do
+      stubs.get("#{path}/#{sub_path}") { |_env| api_response }
+      client.request(sub_path, filters: filters, read_timeout: 30)
+      stubs.get("#{path}/other") do |env|
+        expect(env.request.read_timeout).to eq KB.config.request.read_timeout
+        api_response
+      end
+      client.request('other')
+    end
+
+    it 'passes the override on non-GET requests too' do
+      stubs.post("#{path}/#{sub_path}") do |env|
+        expect(env.request.read_timeout).to eq 30
+        api_response
+      end
+      client.request(sub_path, filters: filters, method: :post, read_timeout: 30)
+    end
+  end
 end
 # rubocop:enable RSpec/MultipleMemoizedHelpers