Merge pull request #232 from RodneyU215/RU_fixing_issue_231

Addressing a few issues with #run_ping! (Fixes #231)

CHANGELOG.md

@@ -1,6 +1,7 @@
 ### 0.13.2 (Next)
 
 * Your contribution here.
+* [#232](https://github.com/slack-ruby/slack-ruby-client/pull/232): Addressing a few issues with #run_ping! (fixes #231) - [@RodneyU215](https://github.com/RodneyU215).
 * [#226](https://github.com/slack-ruby/slack-ruby-client/pull/226): Added periodic ping that reconnects on failure for `async-websocket` - [@RodneyU215](https://github.com/RodneyU215), [@dblock](https://github.com/dblock), [@ioquatix](https://github.com/ioquatix).
 
 ### 0.13.1 (2018/9/30)

lib/slack/real_time/client.rb

@@ -103,31 +103,31 @@ def run_loop
       end
 
       def run_ping!
-        return if websocket_ping.nil? || websocket_ping < 1
-        begin
-          loop do
-            yield websocket_ping if block_given?
-            run_ping
-          end
-        rescue Slack::RealTime::Client::ClientNotStartedError
-          @socket.restart_async(self)
-          retry if started?
-        end
+        time_since_last_message = @socket.time_since_last_message
+        return if time_since_last_message < websocket_ping
+        raise Slack::RealTime::Client::ClientNotStartedError if time_since_last_message > (websocket_ping * 2)
+        ping
+      rescue Slack::RealTime::Client::ClientNotStartedError
+        restart_async
+        retry if started?
+        raise
       end
 
-      def run_ping
-        return if @socket.time_since_last_message < websocket_ping
-
-        if @socket.time_since_last_message > (websocket_ping * 2)
-          @socket.disconnect!
-          @socket.close
-        end
-
-        ping
+      def run_ping?
+        !websocket_ping.nil? && websocket_ping > 0
       end
 
       protected
 
+      def restart_async
+        @socket.close
+        start = web_client.send(rtm_start_method, start_options)
+        data = Slack::Messages::Message.new(start)
+        @url = data.url
+        @store = @store_class.new(data) if @store_class
+        @socket.restart_async(self, @url)
+      end
+
       # @return [Slack::RealTime::Socket]
       def build_socket
         raise ClientAlreadyStartedError if started?
@@ -170,10 +170,7 @@ def send_json(data)
       def open(_event); end
 
       def close(_event)
-        socket = @socket
-        @socket = nil
-
-        [socket, socket_class].each do |s|
+        [@socket, socket_class].each do |s|
           s.close if s.respond_to?(:close)
         end
       end

lib/slack/real_time/concurrency/async.rb

@@ -1,9 +1,14 @@
 require 'async/websocket'
+require 'async/clock'
 
 module Slack
   module RealTime
     module Concurrency
       module Async
+        class Reactor < ::Async::Reactor
+          def_delegators :@timers, :cancel
+        end
+
         class Client < ::Async::WebSocket::Client
           extend ::Forwardable
           def_delegators :@driver, :on, :text, :binary, :emit
@@ -13,29 +18,32 @@ class Socket < Slack::RealTime::Socket
           attr_reader :client
 
           def start_async(client)
+            @reactor = Reactor.new
             Thread.new do
-              ::Async::Reactor.run do |task|
+              @reactor.every(client.websocket_ping) { client.run_ping! } if client.run_ping?
+              @reactor.run do |task|
                 task.async do
                   client.run_loop
                 end
-                task.async do |subtask|
-                  client.run_ping! do |delay|
-                    subtask.sleep delay
-                  end
-                end
               end
             end
           end
 
-          def restart_async(client)
-            ::Async::Reactor.run do
-              client.build_socket
+          def restart_async(client, new_url)
+            @url = new_url
+            return unless @reactor
+            @reactor.async do
               client.run_loop
             end
           end
 
+          def disconnect!
+            super
+            @reactor.cancel
+          end
+
           def current_time
-            Async::Clock.now
+            ::Async::Clock.now
           end
 
           def connect!

lib/slack/real_time/socket.rb

@@ -63,6 +63,7 @@ def restart_async(_client)
       end
 
       def time_since_last_message
+        return 0 unless @last_message_at
         current_time - @last_message_at
       end
 

spec/integration/integration_spec.rb

@@ -118,6 +118,18 @@ def stop_server
     start_server
   end
 
+  # We currently only send regular pings when using 'async-websocket'. See Issue #223.
+  if ENV['CONCURRENCY'] == 'async-websocket'
+    it 'sends pings' do
+      client.websocket_ping = 2
+      client.on :pong do |data|
+        expect(data.reply_to).to be 1
+        client.stop!
+      end
+      start_server
+    end
+  end
+
   it 'gets close, followed by closed' do
     client.on :hello do
       expect(client.started?).to be true

spec/slack/real_time/client_spec.rb

@@ -42,7 +42,7 @@
       end
     end
   end
-  context 'client with a full store', vcr: { cassette_name: 'web/rtm_start' } do
+  context 'client with a full store', vcr: { cassette_name: 'web/rtm_start', allow_playback_repeats: true } do
     let(:client) { Slack::RealTime::Client.new(store_class: Slack::RealTime::Stores::Store) }
     let(:url) { 'wss://ms173.slack-msgs.com/websocket/lqcUiAvrKTP-uuid=' }
     describe '#start!' do
@@ -135,35 +135,26 @@
         allow(socket).to receive(:start_async)
         client.start_async
       end
-      describe '#run_ping' do
-        it 'sends ping messages when the connection is idle' do
+      describe '#run_ping!' do
+        it 'sends ping messages when the websocket connection is idle' do
           allow(socket).to receive(:time_since_last_message).and_return(30)
           expect(socket).to receive(:send_data).with('{"type":"ping","id":1}')
-          client.run_ping
+          client.run_ping!
         end
-        it 'disconnects the websocket when the connection is idle for too long' do
-          allow(socket).to receive(:time_since_last_message).and_return(75)
-          allow(socket).to receive(:connected?).and_return(false)
-
-          expect(socket).to receive(:disconnect!)
+        it 'reconnects the websocket if it has been idle for too long' do
+          allow(socket).to receive(:time_since_last_message).and_return(75, 31)
+          allow(socket).to receive(:connected?).and_return(true)
           expect(socket).to receive(:close)
-          expect { client.run_ping }.to raise_error Slack::RealTime::Client::ClientNotStartedError
-        end
-      end
-      describe '#run_ping!' do
-        it 'returns if websocket_ping is less than 1' do
-          client.websocket_ping = 0
-          expect(client).to_not receive(:run_ping)
+          expect(socket).to receive(:restart_async)
+          expect(socket).to receive(:send_data).with('{"type":"ping","id":1}')
           client.run_ping!
         end
-        it 'reconnects the websocket if an exception is thrown' do
+        it 'raises an exception if reconnect attempts fail' do
           allow(socket).to receive(:time_since_last_message).and_return(75)
-          allow(socket).to receive(:disconnect!)
           allow(socket).to receive(:close)
+          allow(socket).to receive(:restart_async)
           allow(socket).to receive(:connected?).and_return(false)
-
-          expect(socket).to receive(:restart_async)
-          client.run_ping!
+          expect { client.run_ping! }.to raise_error Slack::RealTime::Client::ClientNotStartedError
         end
       end
     end
@@ -281,6 +272,18 @@
         end
       end
     end
+    describe '#run_ping?' do
+      it 'returns true when websocket_ping is greater than 0' do
+        client.websocket_ping = 30
+        expect(client.run_ping?).to be true
+      end
+      it 'returns false when websocket_ping is less than 1' do
+        client.websocket_ping = 0
+        expect(client.run_ping?).to be false
+        client.websocket_ping = nil
+        expect(client.run_ping?).to be false
+      end
+    end
   end
   context 'with custom settings' do
     describe '#initialize' do