Properly retry on timeouts in all strategies

Author: benedikt

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

@@ -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

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

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

@@ -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

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])