Merge pull request #10 from userlist/error-handling

Improve error handling

Author: benedikt

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)
 

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

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

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

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

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

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'

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

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)

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)