diff --git a/CHANGELOG.md b/CHANGELOG.md index 946f103..de952ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,8 @@ ## Unreleased (main) -- Updates ActiveJob Worker to retry on Timeout::Error with polynomially longer wait times, up to 10 attempts +- Updates ActiveJob Worker to retry on errors with polynomially longer wait times, up to 10 attempts +- Improve internal error handling to rely on exceptions ## v0.9.0 (2024-03-19) diff --git a/lib/userlist.rb b/lib/userlist.rb index 55f8f05..efb6e9e 100644 --- a/lib/userlist.rb +++ b/lib/userlist.rb @@ -18,7 +18,7 @@ class ConfigurationError < Error def initialize(key) @key = key.to_sym - super <<~MESSAGE + super(<<~MESSAGE) Missing required configuration value for `#{key}` Please set a value for `#{key}` using an environment variable: @@ -34,6 +34,22 @@ def initialize(key) end end + class TimeoutError < Error; end + + class RequestError < Error + attr_reader :response + + def initialize(response) + super("Request failed with status #{response.code}: #{response.body}") + + @response = response + end + + def status + @response.code.to_i + end + end + class << self def config @config ||= Userlist::Config.new diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb index 7b0ad96..64826a2 100644 --- a/lib/userlist/push/client.rb +++ b/lib/userlist/push/client.rb @@ -49,18 +49,35 @@ def http end def request(method, path, payload = nil) + request = build_request(method, path, payload) + + logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}" + + response = process_request(request) + + logger.debug "Recieved #{response.code} #{response.message} with body #{response.body}" + + handle_response(response) + end + + def build_request(method, path, payload) request = method.new(path) request['Accept'] = 'application/json' request['Authorization'] = "Push #{token}" request['Content-Type'] = 'application/json; charset=UTF-8' request.body = JSON.generate(payload) if payload + request + end - logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}" - + def process_request(request) http.start unless http.started? - response = http.request(request) + http.request(request) + rescue Timeout::Error => e + raise Userlist::TimeoutError, e.message + end - logger.debug "Recieved #{response.code} #{response.message} with body #{response.body}" + def handle_response(response) + raise(Userlist::RequestError, response) if response.code.to_i >= 400 response end diff --git a/lib/userlist/push/strategies/active_job/worker.rb b/lib/userlist/push/strategies/active_job/worker.rb index d4684be..99f5269 100644 --- a/lib/userlist/push/strategies/active_job/worker.rb +++ b/lib/userlist/push/strategies/active_job/worker.rb @@ -5,7 +5,7 @@ class Push module Strategies class ActiveJob class Worker < ::ActiveJob::Base - retry_on Timeout::Error, wait: :polynomially_longer, attempts: 10 + retry_on Userlist::TimeoutError, Userlist::RequestError, wait: :polynomially_longer, attempts: 10 def perform(method, *args) client = Userlist::Push::Client.new diff --git a/lib/userlist/push/strategies/direct.rb b/lib/userlist/push/strategies/direct.rb index adebc52..627dba6 100644 --- a/lib/userlist/push/strategies/direct.rb +++ b/lib/userlist/push/strategies/direct.rb @@ -19,11 +19,7 @@ def client end def retryable - @retryable ||= Userlist::Retryable.new do |response| - status = response.code.to_i - - status >= 500 || status == 429 - end + @retryable ||= Userlist::Retryable.new end end end diff --git a/lib/userlist/push/strategies/threaded/worker.rb b/lib/userlist/push/strategies/threaded/worker.rb index f0dad23..e01975a 100644 --- a/lib/userlist/push/strategies/threaded/worker.rb +++ b/lib/userlist/push/strategies/threaded/worker.rb @@ -46,11 +46,7 @@ def client end def retryable - @retryable ||= Userlist::Retryable.new do |response| - status = response.code.to_i - - status >= 500 || status == 429 - end + @retryable ||= Userlist::Retryable.new end end end diff --git a/lib/userlist/retryable.rb b/lib/userlist/retryable.rb index fc45aeb..9980739 100644 --- a/lib/userlist/retryable.rb +++ b/lib/userlist/retryable.rb @@ -7,12 +7,24 @@ class Retryable MULTIPLIER = 2 MAX_DELAY = 10_000 + DEFAULT_RETRY_CHECK = lambda do |error| + case error + when Userlist::RequestError + status = error.status + status >= 500 || status == 429 + when Userlist::TimeoutError + true + else + false + end + end + def initialize(retries: RETRIES, delay: DELAY, max_delay: MAX_DELAY, multiplier: MULTIPLIER, &retry_check) @retries = retries @delay = delay @max_delay = max_delay @multiplier = multiplier - @retry_check = retry_check + @retry_check = retry_check || DEFAULT_RETRY_CHECK end def retry?(value) @@ -21,15 +33,17 @@ def retry?(value) def attempt (0..@retries).each do |attempt| - if attempt.positive? - milliseconds = delay(attempt) - logger.debug "Retrying in #{milliseconds}ms, #{@retries - attempt} retries left" - sleep(milliseconds / 1000.0) + begin + if attempt.positive? + milliseconds = delay(attempt) + logger.debug "Retrying in #{milliseconds}ms, #{@retries - attempt} retries left" + sleep(milliseconds / 1000.0) + end + + return yield + rescue Userlist::Error => e + raise e unless retry?(e) end - - result = yield - - return result unless retry?(result) end logger.debug 'Retries exhausted, giving up' diff --git a/spec/userlist/push/client_spec.rb b/spec/userlist/push/client_spec.rb index 991134c..d8ed6e4 100644 --- a/spec/userlist/push/client_spec.rb +++ b/spec/userlist/push/client_spec.rb @@ -61,6 +61,20 @@ subject.get('/events') end + + it 'should raise an error when the request fails' do + stub_request(:get, 'https://endpoint.test.local/events') + .to_return(status: 500) + + expect { subject.get('/events') }.to raise_error(Userlist::RequestError) + end + + it 'should raise a timeout error when the request times out' do + stub_request(:get, 'https://endpoint.test.local/events') + .to_timeout + + expect { subject.get('/events') }.to raise_error(Userlist::TimeoutError) + end end describe '#post' do @@ -80,6 +94,20 @@ subject.post('/events', payload) end + + it 'should raise an error when the request fails' do + stub_request(:post, 'https://endpoint.test.local/events') + .to_return(status: 500) + + expect { subject.post('/events') }.to raise_error(Userlist::RequestError) + end + + it 'should raise a timeout error when the request times out' do + stub_request(:post, 'https://endpoint.test.local/events') + .to_timeout + + expect { subject.post('/events') }.to raise_error(Userlist::TimeoutError) + end end describe '#put' do @@ -99,6 +127,20 @@ subject.put('/events', payload) end + + it 'should raise an error when the request fails' do + stub_request(:put, 'https://endpoint.test.local/events') + .to_return(status: 500) + + expect { subject.put('/events') }.to raise_error(Userlist::RequestError) + end + + it 'should raise a timeout error when the request times out' do + stub_request(:put, 'https://endpoint.test.local/events') + .to_timeout + + expect { subject.put('/events') }.to raise_error(Userlist::TimeoutError) + end end describe '#delete' do @@ -118,5 +160,19 @@ subject.delete('/events', payload) end + + it 'should raise an error when the request fails' do + stub_request(:delete, 'https://endpoint.test.local/events') + .to_return(status: 500) + + expect { subject.delete('/events') }.to raise_error(Userlist::RequestError) + end + + it 'should raise a timeout error when the request times out' do + stub_request(:delete, 'https://endpoint.test.local/events') + .to_timeout + + expect { subject.delete('/events') }.to raise_error(Userlist::TimeoutError) + end end end diff --git a/spec/userlist/push/strategies/active_job/worker_spec.rb b/spec/userlist/push/strategies/active_job/worker_spec.rb index 43049f3..790ab85 100644 --- a/spec/userlist/push/strategies/active_job/worker_spec.rb +++ b/spec/userlist/push/strategies/active_job/worker_spec.rb @@ -30,7 +30,14 @@ end it 'should retry the job on failure' do - allow(client).to receive(:post).and_raise(Timeout::Error) + allow(client).to receive(:post).and_raise(Userlist::RequestError.new(double(code: '500', body: 'Internal Error'))) + + expect { described_class.perform_now('post', '/events', payload) } + .to change(described_class.queue_adapter.enqueued_jobs, :size).by(1) + end + + it 'should retry the job on timeouts' do + allow(client).to receive(:post).and_raise(Userlist::TimeoutError) expect { described_class.perform_now('post', '/events', payload) } .to change(described_class.queue_adapter.enqueued_jobs, :size).by(1) diff --git a/spec/userlist/push/strategies/direct_spec.rb b/spec/userlist/push/strategies/direct_spec.rb index b2031dd..2db3e02 100644 --- a/spec/userlist/push/strategies/direct_spec.rb +++ b/spec/userlist/push/strategies/direct_spec.rb @@ -32,7 +32,16 @@ it 'should retry failed responses' do payload = { foo: :bar } - expect(client).to receive(:post) { double(code: '500') }.exactly(11).times + expect(client).to receive(:post) { raise Userlist::RequestError, double(code: '500', body: 'Internal Error') }.exactly(11).times + expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times + + subject.call(:post, '/users', payload) + end + + it 'should retry on timeouts' do + payload = { foo: :bar } + + expect(client).to receive(:post) { raise Userlist::TimeoutError }.exactly(11).times expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times subject.call(:post, '/users', payload) diff --git a/spec/userlist/push/strategies/threaded/worker_spec.rb b/spec/userlist/push/strategies/threaded/worker_spec.rb index 08a9728..c2fb92c 100644 --- a/spec/userlist/push/strategies/threaded/worker_spec.rb +++ b/spec/userlist/push/strategies/threaded/worker_spec.rb @@ -45,7 +45,16 @@ it 'should retry failed responses' do payload = { foo: :bar } - expect(client).to receive(:post) { double(code: '500') }.exactly(11).times + expect(client).to receive(:post) { raise Userlist::RequestError, double(code: '500', body: 'Internal Error') }.exactly(11).times + expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times + + queue.push([:post, '/users', payload]) + end + + it 'should retry on timeouts' do + payload = { foo: :bar } + + expect(client).to receive(:post) { raise Userlist::TimeoutError }.exactly(11).times expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times queue.push([:post, '/users', payload]) diff --git a/spec/userlist/retryable_spec.rb b/spec/userlist/retryable_spec.rb index d641853..386d8e8 100644 --- a/spec/userlist/retryable_spec.rb +++ b/spec/userlist/retryable_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' RSpec.describe Userlist::Retryable do - subject { described_class.new(&:!) } + subject { described_class.new { true } } before do allow_any_instance_of(described_class).to receive(:sleep) @@ -12,7 +12,6 @@ subject.attempt do attempts += 1 - true end expect(attempts).to eq(1) @@ -23,7 +22,8 @@ subject.attempt do attempts += 1 - attempts > 5 + + raise Userlist::Error unless attempts > 5 end expect(attempts).to eq(6) @@ -34,7 +34,8 @@ subject.attempt do attempts += 1 - false + + raise Userlist::Error end expect(attempts).to eq(11) @@ -43,6 +44,6 @@ it 'should wait between the attempts' do expect(subject).to receive(:sleep).exactly(10).times - subject.attempt { false } + subject.attempt { raise Userlist::Error } end end