From f056db1a51a1427dcbea3ab16e160be1c03008c9 Mon Sep 17 00:00:00 2001 From: Rodney Urquhart Date: Wed, 17 Oct 2018 17:17:34 -0700 Subject: [PATCH 1/4] WIP: Adding require statement and ensuring that everything runs on one Reactor --- lib/slack/real_time/client.rb | 13 ++++++++++--- lib/slack/real_time/concurrency/async.rb | 20 +++++++++++++++----- lib/slack/real_time/socket.rb | 1 + 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/slack/real_time/client.rb b/lib/slack/real_time/client.rb index f3d34f4c..4193da2f 100644 --- a/lib/slack/real_time/client.rb +++ b/lib/slack/real_time/client.rb @@ -110,7 +110,7 @@ def run_ping! run_ping end rescue Slack::RealTime::Client::ClientNotStartedError - @socket.restart_async(self) + restart_async retry if started? end end @@ -119,13 +119,20 @@ 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 + raise Slack::RealTime::Client::ClientNotStartedError end ping end + def restart_async + 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 + protected # @return [Slack::RealTime::Socket] diff --git a/lib/slack/real_time/concurrency/async.rb b/lib/slack/real_time/concurrency/async.rb index d2696591..6feab5b0 100644 --- a/lib/slack/real_time/concurrency/async.rb +++ b/lib/slack/real_time/concurrency/async.rb @@ -1,4 +1,5 @@ require 'async/websocket' +require 'async/clock' module Slack module RealTime @@ -13,8 +14,9 @@ class Socket < Slack::RealTime::Socket attr_reader :client def start_async(client) + @reactor = ::Async::Reactor.new Thread.new do - ::Async::Reactor.run do |task| + @reactor.run do |task| task.async do client.run_loop end @@ -27,15 +29,16 @@ def start_async(client) 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.run do client.run_loop end end def current_time - Async::Clock.now + ::Async::Clock.now end def connect! @@ -43,6 +46,13 @@ def connect! run_loop end + def disconnect! + super + if @reactor + @reactor.close + end + end + def close @closing = true @driver.close if @driver diff --git a/lib/slack/real_time/socket.rb b/lib/slack/real_time/socket.rb index 36dad5c4..62ed31ff 100644 --- a/lib/slack/real_time/socket.rb +++ b/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 From 389e029c0b0d54a6be45cea469d7f8dc5c703b68 Mon Sep 17 00:00:00 2001 From: Rodney Urquhart Date: Thu, 18 Oct 2018 04:39:59 -0700 Subject: [PATCH 2/4] Keeping main Thread/Reactor running and canceling timers on #disconnect! --- lib/slack/real_time/client.rb | 35 +++++++++--------------- lib/slack/real_time/concurrency/async.rb | 26 ++++++++---------- spec/slack/real_time/client_spec.rb | 30 +++++--------------- 3 files changed, 32 insertions(+), 59 deletions(-) diff --git a/lib/slack/real_time/client.rb b/lib/slack/real_time/client.rb index 4193da2f..6e9d4c8c 100644 --- a/lib/slack/real_time/client.rb +++ b/lib/slack/real_time/client.rb @@ -103,29 +103,23 @@ 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 - restart_async - 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) - raise Slack::RealTime::Client::ClientNotStartedError - end - - ping + def run_ping? + !websocket_ping.nil? && websocket_ping > 0 end def restart_async + return unless @socket + @socket.close start = web_client.send(rtm_start_method, start_options) data = Slack::Messages::Message.new(start) @url = data.url @@ -177,10 +171,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 diff --git a/lib/slack/real_time/concurrency/async.rb b/lib/slack/real_time/concurrency/async.rb index 6feab5b0..5029ed02 100644 --- a/lib/slack/real_time/concurrency/async.rb +++ b/lib/slack/real_time/concurrency/async.rb @@ -5,6 +5,10 @@ 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 @@ -14,17 +18,13 @@ class Socket < Slack::RealTime::Socket attr_reader :client def start_async(client) - @reactor = ::Async::Reactor.new + @reactor = Reactor.new Thread.new do + @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 @@ -32,11 +32,16 @@ def start_async(client) def restart_async(client, new_url) @url = new_url return unless @reactor - @reactor.run do + @reactor.async do client.run_loop end end + def disconnect! + super + @reactor.cancel + end + def current_time ::Async::Clock.now end @@ -46,13 +51,6 @@ def connect! run_loop end - def disconnect! - super - if @reactor - @reactor.close - end - end - def close @closing = true @driver.close if @driver diff --git a/spec/slack/real_time/client_spec.rb b/spec/slack/real_time/client_spec.rb index face22a2..39971e42 100644 --- a/spec/slack/real_time/client_spec.rb +++ b/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,34 +135,18 @@ allow(socket).to receive(:start_async) client.start_async end - describe '#run_ping' do + describe '#run_ping!' do it 'sends ping messages when the 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 - 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!) - 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) client.run_ping! end - it 'reconnects the websocket if an exception is thrown' 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(:connected?).and_return(false) - + 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(socket).to receive(:restart_async) + expect(socket).to receive(:send_data).with('{"type":"ping","id":1}') client.run_ping! end end From 928a124073251ee87a64b2c631e5de5ee1985ed6 Mon Sep 17 00:00:00 2001 From: Rodney Urquhart Date: Thu, 18 Oct 2018 12:03:30 -0700 Subject: [PATCH 3/4] Adding integration test for ping and making restart a protected method. --- lib/slack/real_time/client.rb | 5 ++--- spec/integration/integration_spec.rb | 12 ++++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/slack/real_time/client.rb b/lib/slack/real_time/client.rb index 6e9d4c8c..174cdab2 100644 --- a/lib/slack/real_time/client.rb +++ b/lib/slack/real_time/client.rb @@ -117,8 +117,9 @@ def run_ping? !websocket_ping.nil? && websocket_ping > 0 end + protected + def restart_async - return unless @socket @socket.close start = web_client.send(rtm_start_method, start_options) data = Slack::Messages::Message.new(start) @@ -127,8 +128,6 @@ def restart_async @socket.restart_async(self, @url) end - protected - # @return [Slack::RealTime::Socket] def build_socket raise ClientAlreadyStartedError if started? diff --git a/spec/integration/integration_spec.rb b/spec/integration/integration_spec.rb index f44dc5a0..bb061aaf 100644 --- a/spec/integration/integration_spec.rb +++ b/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 From 036b0adebf19611dc6e1a3ecf9ecd44831ecfb9b Mon Sep 17 00:00:00 2001 From: Rodney Urquhart Date: Fri, 19 Oct 2018 07:36:39 -0700 Subject: [PATCH 4/4] Adding more unit tests and updating CHANGELOG --- CHANGELOG.md | 1 + spec/slack/real_time/client_spec.rb | 21 ++++++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31b25de2..f8fe2116 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/spec/slack/real_time/client_spec.rb b/spec/slack/real_time/client_spec.rb index 39971e42..e9ac1328 100644 --- a/spec/slack/real_time/client_spec.rb +++ b/spec/slack/real_time/client_spec.rb @@ -136,7 +136,7 @@ client.start_async end describe '#run_ping!' do - it 'sends ping messages when the connection is idle' 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! @@ -149,6 +149,13 @@ expect(socket).to receive(:send_data).with('{"type":"ping","id":1}') client.run_ping! end + it 'raises an exception if reconnect attempts fail' do + allow(socket).to receive(:time_since_last_message).and_return(75) + allow(socket).to receive(:close) + allow(socket).to receive(:restart_async) + allow(socket).to receive(:connected?).and_return(false) + expect { client.run_ping! }.to raise_error Slack::RealTime::Client::ClientNotStartedError + end end end end @@ -265,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