Fix #78: ping thread terminates on a failed restart.

Author: dblock

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 #### 0.8.1 (Next)
 
+* [#79](https://github.com/slack-ruby/slack-ruby-bot-server/pull/79): Fix: ping worker terminates on a failed restart - [@dblock](https://github.com/dblock).
 * Your contribution here.
 
 #### 0.8.0 (2018/9/8)

lib/slack-ruby-bot-server/ping.rb

@@ -68,32 +68,55 @@ def down!
       @error_count += 1
       return true if retries_left?
       restart!
-      false
     end
 
     def restart!
       logger.warn "RESTART: #{owner}"
-      begin
-        connection.close
-      rescue Async::Wrapper::Cancelled
-        # ignore, from connection.close
-      end
+      close_connection
+      close_driver
+      emit_close
+      false
+    rescue StandardError => e
+      logger.warn "Error restarting team #{owner.id}: #{e.message}."
+      true
+    end
+
+    def close_connection
+      return unless connection
+      connection.close
+    rescue Async::Wrapper::Cancelled
+      # ignore, from connection.close
+    rescue StandardError => e
+      logger.warn "Error closing connection for #{owner.id}: #{e.message}."
+      raise e
+    end
+
+    def close_driver
+      return unless driver
       driver.close
+    rescue StandardError => e
+      logger.warn "Error closing driver for #{owner.id}: #{e.message}."
+      raise e
+    end
+
+    def emit_close
+      return unless driver
       driver.emit(:close, WebSocket::Driver::CloseEvent.new(1001, 'bot offline'))
     rescue StandardError => e
-      logger.warn "Error restarting team #{owner.id}: #{e.message}."
+      logger.warn "Error sending :close event to driver for #{owner.id}: #{e.message}."
+      raise e
     end
 
     def ping_interval
       options[:ping_interval] || 60
     end
 
     def retries_left?
-      retries_left >= 0
+      retry_count - error_count >= 0
     end
 
     def retries_left
-      retry_count - error_count
+      [0, retry_count - error_count].max
     end
 
     def retry_count

spec/slack-ruby-bot-server/ping_spec.rb

@@ -60,16 +60,43 @@
           expect(subject.send(:error_count)).to eq 0
         end
       end
+
+      context 'after two more failed checks' do
+        before do
+          allow(subject).to receive(:online?).and_return(false).twice
+          2.times { subject.send(:check!) }
+        end
+
+        it 'does not decrement retries left below zero' do
+          expect(subject.send(:retries_left)).to eq 0
+        end
+
+        it 'sets error count' do
+          expect(subject.send(:error_count)).to eq 3
+        end
+      end
     end
 
     it 'terminates the ping worker after account_inactive' do
       allow(subject).to receive(:online?).and_raise('account_inactive')
       subject.start!
     end
 
-    it 'terminates after a number of retries' do
+    it 'restarts and terminates after a number of retries' do
       allow(subject).to receive(:online?).and_return(false)
       expect(subject).to receive(:check!).exactly(3).times.and_call_original
+      expect(subject).to receive(:restart!).and_call_original
+      expect(subject).to receive(:close_connection).and_call_original
+      expect(subject).to receive(:close_driver).and_call_original
+      expect(subject).to receive(:emit_close).and_call_original
+      subject.start!
+    end
+
+    it 'does not terminate upon a failed restart' do
+      allow(subject).to receive(:online?).and_return(false)
+      allow(subject).to receive(:close_connection) { raise 'error closing connection' }
+      expect(subject).to receive(:check!).exactly(3).times.and_call_original
+      expect(subject).to receive(:check!).and_return(false)
       subject.start!
     end
   end