From 8a83075bec2fc721995674302d0840b5cf7e044d Mon Sep 17 00:00:00 2001 From: Benedikt Deicke Date: Fri, 15 Nov 2024 14:09:30 +0100 Subject: [PATCH 1/7] Raise errors from the client and handle it in the Retryable --- lib/userlist.rb | 16 ++++++++++- lib/userlist/push/client.rb | 2 ++ lib/userlist/push/strategies/direct.rb | 5 ++-- .../push/strategies/threaded/worker.rb | 5 ++-- lib/userlist/retryable.rb | 18 ++++++------ spec/userlist/push/client_spec.rb | 28 +++++++++++++++++++ spec/userlist/push/strategies/direct_spec.rb | 2 +- .../push/strategies/threaded/worker_spec.rb | 2 +- spec/userlist/retryable_spec.rb | 11 ++++---- 9 files changed, 69 insertions(+), 20 deletions(-) diff --git a/lib/userlist.rb b/lib/userlist.rb index 55f8f05..33914b6 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,20 @@ def initialize(key) end 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..3f5d9e6 100644 --- a/lib/userlist/push/client.rb +++ b/lib/userlist/push/client.rb @@ -62,6 +62,8 @@ def request(method, path, payload = nil) logger.debug "Recieved #{response.code} #{response.message} with body #{response.body}" + raise(Userlist::RequestError, response) if response.code.to_i >= 400 + response end diff --git a/lib/userlist/push/strategies/direct.rb b/lib/userlist/push/strategies/direct.rb index adebc52..5b20646 100644 --- a/lib/userlist/push/strategies/direct.rb +++ b/lib/userlist/push/strategies/direct.rb @@ -19,9 +19,10 @@ def client end def retryable - @retryable ||= Userlist::Retryable.new do |response| - status = response.code.to_i + @retryable ||= Userlist::Retryable.new do |error| + next false unless error.is_a?(Userlist::RequestError) + status = error.status status >= 500 || status == 429 end end diff --git a/lib/userlist/push/strategies/threaded/worker.rb b/lib/userlist/push/strategies/threaded/worker.rb index f0dad23..273715c 100644 --- a/lib/userlist/push/strategies/threaded/worker.rb +++ b/lib/userlist/push/strategies/threaded/worker.rb @@ -46,9 +46,10 @@ def client end def retryable - @retryable ||= Userlist::Retryable.new do |response| - status = response.code.to_i + @retryable ||= Userlist::Retryable.new do |error| + next false unless error.is_a?(Userlist::RequestError) + status = error.status status >= 500 || status == 429 end end diff --git a/lib/userlist/retryable.rb b/lib/userlist/retryable.rb index fc45aeb..2c3ae1a 100644 --- a/lib/userlist/retryable.rb +++ b/lib/userlist/retryable.rb @@ -21,15 +21,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 StandardError => 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..7c81bf3 100644 --- a/spec/userlist/push/client_spec.rb +++ b/spec/userlist/push/client_spec.rb @@ -61,6 +61,13 @@ 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 end describe '#post' do @@ -80,6 +87,13 @@ 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 end describe '#put' do @@ -99,6 +113,13 @@ 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 end describe '#delete' do @@ -118,5 +139,12 @@ 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 end end diff --git a/spec/userlist/push/strategies/direct_spec.rb b/spec/userlist/push/strategies/direct_spec.rb index b2031dd..006941b 100644 --- a/spec/userlist/push/strategies/direct_spec.rb +++ b/spec/userlist/push/strategies/direct_spec.rb @@ -32,7 +32,7 @@ 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) diff --git a/spec/userlist/push/strategies/threaded/worker_spec.rb b/spec/userlist/push/strategies/threaded/worker_spec.rb index 08a9728..8847df0 100644 --- a/spec/userlist/push/strategies/threaded/worker_spec.rb +++ b/spec/userlist/push/strategies/threaded/worker_spec.rb @@ -45,7 +45,7 @@ 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]) diff --git a/spec/userlist/retryable_spec.rb b/spec/userlist/retryable_spec.rb index d641853..47a827f 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 unless attempts > 5 end expect(attempts).to eq(6) @@ -34,7 +34,8 @@ subject.attempt do attempts += 1 - false + + raise 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 } end end From d7b44fce7fbd9b9c8c1f454a75f629d775a826c3 Mon Sep 17 00:00:00 2001 From: Benedikt Deicke Date: Fri, 15 Nov 2024 14:13:16 +0100 Subject: [PATCH 2/7] Break request method into smaller chunks --- lib/userlist/push/client.rb | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb index 3f5d9e6..5d6e3af 100644 --- a/lib/userlist/push/client.rb +++ b/lib/userlist/push/client.rb @@ -49,19 +49,32 @@ 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) - - logger.debug "Recieved #{response.code} #{response.message} with body #{response.body}" + http.request(request) + end + def handle_response(response) raise(Userlist::RequestError, response) if response.code.to_i >= 400 response From 977182cb478bdf6e882a90603a49b8984fe3a915 Mon Sep 17 00:00:00 2001 From: Benedikt Deicke Date: Fri, 15 Nov 2024 14:16:24 +0100 Subject: [PATCH 3/7] Properly wrap timeout errors in Userlist::TimeoutError --- lib/userlist.rb | 2 ++ lib/userlist/push/client.rb | 2 ++ spec/userlist/push/client_spec.rb | 28 ++++++++++++++++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/lib/userlist.rb b/lib/userlist.rb index 33914b6..efb6e9e 100644 --- a/lib/userlist.rb +++ b/lib/userlist.rb @@ -34,6 +34,8 @@ def initialize(key) end end + class TimeoutError < Error; end + class RequestError < Error attr_reader :response diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb index 5d6e3af..64826a2 100644 --- a/lib/userlist/push/client.rb +++ b/lib/userlist/push/client.rb @@ -72,6 +72,8 @@ def build_request(method, path, payload) def process_request(request) http.start unless http.started? http.request(request) + rescue Timeout::Error => e + raise Userlist::TimeoutError, e.message end def handle_response(response) diff --git a/spec/userlist/push/client_spec.rb b/spec/userlist/push/client_spec.rb index 7c81bf3..d8ed6e4 100644 --- a/spec/userlist/push/client_spec.rb +++ b/spec/userlist/push/client_spec.rb @@ -68,6 +68,13 @@ 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 @@ -94,6 +101,13 @@ 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 @@ -120,6 +134,13 @@ 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 @@ -146,5 +167,12 @@ 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 From 0b107e57271599746c49debf57fddcca573d5ffd Mon Sep 17 00:00:00 2001 From: Benedikt Deicke Date: Fri, 15 Nov 2024 14:25:41 +0100 Subject: [PATCH 4/7] Properly retry on timeouts in all strategies --- lib/userlist/push/strategies/active_job/worker.rb | 2 +- lib/userlist/push/strategies/direct.rb | 13 +++++++++---- lib/userlist/push/strategies/threaded/worker.rb | 13 +++++++++---- .../push/strategies/active_job/worker_spec.rb | 9 ++++++++- spec/userlist/push/strategies/direct_spec.rb | 9 +++++++++ .../push/strategies/threaded/worker_spec.rb | 9 +++++++++ 6 files changed, 45 insertions(+), 10 deletions(-) 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 5b20646..088681d 100644 --- a/lib/userlist/push/strategies/direct.rb +++ b/lib/userlist/push/strategies/direct.rb @@ -20,10 +20,15 @@ def client def retryable @retryable ||= Userlist::Retryable.new do |error| - next false unless error.is_a?(Userlist::RequestError) - - status = error.status - status >= 500 || status == 429 + case error + when Userlist::RequestError + status = error.status + status >= 500 || status == 429 + when Userlist::TimeoutError + true + else + false + end end end end diff --git a/lib/userlist/push/strategies/threaded/worker.rb b/lib/userlist/push/strategies/threaded/worker.rb index 273715c..8a054d3 100644 --- a/lib/userlist/push/strategies/threaded/worker.rb +++ b/lib/userlist/push/strategies/threaded/worker.rb @@ -47,10 +47,15 @@ def client def retryable @retryable ||= Userlist::Retryable.new do |error| - next false unless error.is_a?(Userlist::RequestError) - - status = error.status - status >= 500 || status == 429 + case error + when Userlist::RequestError + status = error.status + status >= 500 || status == 429 + when Userlist::TimeoutError + true + else + false + end 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 006941b..2db3e02 100644 --- a/spec/userlist/push/strategies/direct_spec.rb +++ b/spec/userlist/push/strategies/direct_spec.rb @@ -37,5 +37,14 @@ 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) + end end end diff --git a/spec/userlist/push/strategies/threaded/worker_spec.rb b/spec/userlist/push/strategies/threaded/worker_spec.rb index 8847df0..c2fb92c 100644 --- a/spec/userlist/push/strategies/threaded/worker_spec.rb +++ b/spec/userlist/push/strategies/threaded/worker_spec.rb @@ -51,6 +51,15 @@ 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]) + end + it 'should log failed requests' do allow(client).to receive(:post).and_raise(StandardError) queue.push([:post, '/events', payload]) From f0831dcb147907f37252ab456164fe57e0b9afc8 Mon Sep 17 00:00:00 2001 From: Benedikt Deicke Date: Fri, 15 Nov 2024 14:29:23 +0100 Subject: [PATCH 5/7] Only handle retries for Userlist related errors --- lib/userlist/retryable.rb | 2 +- spec/userlist/retryable_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/userlist/retryable.rb b/lib/userlist/retryable.rb index 2c3ae1a..41e5edd 100644 --- a/lib/userlist/retryable.rb +++ b/lib/userlist/retryable.rb @@ -29,7 +29,7 @@ def attempt end return yield - rescue StandardError => e + rescue Userlist::Error => e raise e unless retry?(e) end end diff --git a/spec/userlist/retryable_spec.rb b/spec/userlist/retryable_spec.rb index 47a827f..386d8e8 100644 --- a/spec/userlist/retryable_spec.rb +++ b/spec/userlist/retryable_spec.rb @@ -23,7 +23,7 @@ subject.attempt do attempts += 1 - raise unless attempts > 5 + raise Userlist::Error unless attempts > 5 end expect(attempts).to eq(6) @@ -35,7 +35,7 @@ subject.attempt do attempts += 1 - raise + raise Userlist::Error end expect(attempts).to eq(11) @@ -44,6 +44,6 @@ it 'should wait between the attempts' do expect(subject).to receive(:sleep).exactly(10).times - subject.attempt { raise } + subject.attempt { raise Userlist::Error } end end From 3dfb7ea06d8f55a3a207826df0fd374fc6503b18 Mon Sep 17 00:00:00 2001 From: Benedikt Deicke Date: Fri, 15 Nov 2024 14:35:10 +0100 Subject: [PATCH 6/7] =?UTF-8?q?Don=E2=80=99t=20require=20a=20custom=20retr?= =?UTF-8?q?y=20check=20block=20on=20all=20Retryable=20instances?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/userlist/push/strategies/direct.rb | 12 +----------- lib/userlist/push/strategies/threaded/worker.rb | 12 +----------- lib/userlist/retryable.rb | 14 +++++++++++++- 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/lib/userlist/push/strategies/direct.rb b/lib/userlist/push/strategies/direct.rb index 088681d..627dba6 100644 --- a/lib/userlist/push/strategies/direct.rb +++ b/lib/userlist/push/strategies/direct.rb @@ -19,17 +19,7 @@ def client end def retryable - @retryable ||= Userlist::Retryable.new do |error| - case error - when Userlist::RequestError - status = error.status - status >= 500 || status == 429 - when Userlist::TimeoutError - true - else - false - end - 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 8a054d3..e01975a 100644 --- a/lib/userlist/push/strategies/threaded/worker.rb +++ b/lib/userlist/push/strategies/threaded/worker.rb @@ -46,17 +46,7 @@ def client end def retryable - @retryable ||= Userlist::Retryable.new do |error| - case error - when Userlist::RequestError - status = error.status - status >= 500 || status == 429 - when Userlist::TimeoutError - true - else - false - end - end + @retryable ||= Userlist::Retryable.new end end end diff --git a/lib/userlist/retryable.rb b/lib/userlist/retryable.rb index 41e5edd..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) From 9d16e8ab43701f7e11457f6a2e5b0d960a97387a Mon Sep 17 00:00:00 2001 From: Benedikt Deicke Date: Fri, 15 Nov 2024 14:36:56 +0100 Subject: [PATCH 7/7] Add changelog entry --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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)