diff --git a/CHANGELOG.md b/CHANGELOG.md index f15a8f46..a2db218b 100644 --- a/CHANGELOG.md +++ b/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`). diff --git a/Gemfile.lock b/Gemfile.lock index f04cbe60..c722bf19 100644 --- a/Gemfile.lock +++ b/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 diff --git a/README.md b/README.md index 2e906dcc..fcb16872 100644 --- a/README.md +++ b/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 🧍🏾 diff --git a/lib/kb/client.rb b/lib/kb/client.rb index 58fd9c2c..44e94de0 100644 --- a/lib/kb/client.rb +++ b/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, diff --git a/lib/kb/version.rb b/lib/kb/version.rb index f1201a2d..56a78b3c 100644 --- a/lib/kb/version.rb +++ b/lib/kb/version.rb @@ -1,3 +1,3 @@ module KB - VERSION = '1.0.0'.freeze + VERSION = '1.1.0'.freeze end diff --git a/spec/client_read_timeout_socket_spec.rb b/spec/client_read_timeout_socket_spec.rb new file mode 100644 index 00000000..c87323a6 --- /dev/null +++ b/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 diff --git a/spec/client_spec.rb b/spec/client_spec.rb index 91b135bb..e2c732db 100644 --- a/spec/client_spec.rb +++ b/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