From c504ff2785244512ee36928637414d01a92afa57 Mon Sep 17 00:00:00 2001 From: dblock Date: Mon, 20 Aug 2018 23:01:39 -0400 Subject: [PATCH 1/6] Added server ping worker. --- lib/slack-ruby-bot-server.rb | 1 + lib/slack-ruby-bot-server/ping.rb | 93 +++++++++++++++++++++++++++++ lib/slack-ruby-bot-server/server.rb | 11 +++- 3 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 lib/slack-ruby-bot-server/ping.rb diff --git a/lib/slack-ruby-bot-server.rb b/lib/slack-ruby-bot-server.rb index 5e130f8..d76600d 100644 --- a/lib/slack-ruby-bot-server.rb +++ b/lib/slack-ruby-bot-server.rb @@ -3,6 +3,7 @@ require 'grape-swagger' require 'slack-ruby-bot' require 'slack-ruby-bot-server/server' +require 'slack-ruby-bot-server/ping' require 'slack-ruby-bot-server/config' require 'slack-ruby-bot-server/ext' diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb new file mode 100644 index 0000000..9a127f9 --- /dev/null +++ b/lib/slack-ruby-bot-server/ping.rb @@ -0,0 +1,93 @@ +module SlackRubyBotServer + class Ping + include Celluloid + + attr_reader :client + attr_reader :options + attr_reader :error_count + + def initialize(client, options = {}) + @options = options + @client = client + @error_count = 0 + logger.level = Logger::INFO + end + + def start! + every ping_interval do + begin + check! + rescue StandardError => e + logger.warn "Error pinging team #{owner.id}: #{e.message}." + end + end + end + + private + + def check! + return if online? + down! + end + + def offline? + !online? + end + + def online? + presence.online + end + + def presence + owner.ping![:presence] + end + + def down! + logger.warn "DOWN: #{owner}, #{retries_left} #{retries_left == 1 ? 'retry' : 'retries'} left" + @error_count += 1 + return if retries_left? + restart! + end + + def restart! + logger.warn "RESTART: #{owner}" + driver.emit(:close, WebSocket::Driver::CloseEvent.new(1001, 'bot offline')) + terminate + end + + def ping_interval + options[:ping_interval] || 3 * 60 + end + + def retries_left? + retries_left >= 0 + end + + def retries_left + retry_count - error_count + end + + def retry_count + options[:retry_count] || 2 + end + + def socket + client.instance_variable_get(:@socket) + end + + def driver + socket&.instance_variable_get(:@driver) + end + + def logger + @logger ||= begin + $stdout.sync = true + Logger.new(STDOUT) + end + end + + def owner + client.owner + end + end +end diff --git a/lib/slack-ruby-bot-server/server.rb b/lib/slack-ruby-bot-server/server.rb index 2d1dc50..3c9c43a 100644 --- a/lib/slack-ruby-bot-server/server.rb +++ b/lib/slack-ruby-bot-server/server.rb @@ -10,7 +10,7 @@ def initialize(attrs = {}) raise 'Missing team' unless @team attrs[:token] = @team.token super(attrs) - client.owner = @team + open! end def restart!(wait = 1) @@ -18,7 +18,16 @@ def restart!(wait = 1) # it would keep retrying without checking for account_inactive or such, we want to restart via service which will disable an inactive team logger.info "#{team.name}: socket closed, restarting ..." SlackRubyBotServer::Service.instance.restart! team, self, wait + open! + end + + private + + def open! client.owner = team + client.on :open do |_event| + SlackRubyBotServer::Ping.new(client).start! + end end end end From c6d4f79d2014d3231425648ce1938dc57fa49f45 Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 21 Aug 2018 17:10:11 -0400 Subject: [PATCH 2/6] Handle account_inactive and invalid_auth in ping. --- lib/slack-ruby-bot-server/ping.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index 9a127f9..7dd1e74 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -15,11 +15,7 @@ def initialize(client, options = {}) def start! every ping_interval do - begin - check! - rescue StandardError => e - logger.warn "Error pinging team #{owner.id}: #{e.message}." - end + check! end end @@ -28,6 +24,14 @@ def start! def check! return if online? down! + rescue StandardError => e + case e.message + when 'account_inactive', 'invalid_auth' then + logger.warn "Error pinging team #{owner.id}: #{e.message}, terminating." + terminate + else + logger.warn "Error pinging team #{owner.id}: #{e.message}." + end end def offline? @@ -72,11 +76,11 @@ def retry_count end def socket - client.instance_variable_get(:@socket) + client.instance_variable_get(:@socket) if client end def driver - socket&.instance_variable_get(:@driver) + socket.instance_variable_get(:@driver) if socket end def logger From 2f346556ea1cf7c83b9858c5a7b5ec438565fd02 Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 21 Aug 2018 17:13:34 -0400 Subject: [PATCH 3/6] Shorten ping interval and reset error count. --- lib/slack-ruby-bot-server/ping.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index 7dd1e74..1411d2a 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -10,7 +10,6 @@ def initialize(client, options = {}) @options = options @client = client @error_count = 0 - logger.level = Logger::INFO end def start! @@ -22,8 +21,11 @@ def start! private def check! - return if online? - down! + if online? + @error_count = 0 + else + down! + end rescue StandardError => e case e.message when 'account_inactive', 'invalid_auth' then @@ -60,7 +62,7 @@ def restart! end def ping_interval - options[:ping_interval] || 3 * 60 + options[:ping_interval] || 60 end def retries_left? From 76cd53e8df7e5c6c6eae33db1d27fdc2b6b05efb Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 21 Aug 2018 17:17:00 -0400 Subject: [PATCH 4/6] Use STDOUT instead of global . --- lib/slack-ruby-bot-server/api/middleware.rb | 2 +- lib/slack-ruby-bot-server/app.rb | 2 +- lib/slack-ruby-bot-server/ping.rb | 2 +- tasks/db.rake | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/slack-ruby-bot-server/api/middleware.rb b/lib/slack-ruby-bot-server/api/middleware.rb index 028f723..34b948f 100644 --- a/lib/slack-ruby-bot-server/api/middleware.rb +++ b/lib/slack-ruby-bot-server/api/middleware.rb @@ -8,7 +8,7 @@ module Api class Middleware def self.logger @logger ||= begin - $stdout.sync = true + STDOUT.sync = true Logger.new(STDOUT) end end diff --git a/lib/slack-ruby-bot-server/app.rb b/lib/slack-ruby-bot-server/app.rb index 22aaed7..8891ddb 100644 --- a/lib/slack-ruby-bot-server/app.rb +++ b/lib/slack-ruby-bot-server/app.rb @@ -18,7 +18,7 @@ def self.instance def logger @logger ||= begin - $stdout.sync = true + STDOUT.sync = true Logger.new(STDOUT) end end diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index 1411d2a..30850de 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -87,7 +87,7 @@ def driver def logger @logger ||= begin - $stdout.sync = true + STDOUT.sync = true Logger.new(STDOUT) end end diff --git a/tasks/db.rake b/tasks/db.rake index faf74c9..200b093 100644 --- a/tasks/db.rake +++ b/tasks/db.rake @@ -1,7 +1,7 @@ namespace :db do def logger @logger ||= begin - $stdout.sync = true + STDOUT.sync = true Logger.new(STDOUT) end end From edc8647599e7a03b882485bb9432745156fd8d6c Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 21 Aug 2018 19:09:18 -0400 Subject: [PATCH 5/6] Tests and documentation for the ping worker. --- CHANGELOG.md | 3 +- README.md | 4 + lib/slack-ruby-bot-server/ping.rb | 2 +- lib/slack-ruby-bot-server/version.rb | 2 +- spec/slack-ruby-bot-server/ping_spec.rb | 101 ++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 spec/slack-ruby-bot-server/ping_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fe996b..504f29a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,11 @@ ### Changelog -#### 0.6.2 (Next) +#### 0.7.0 (Next) * [#60](https://github.com/slack-ruby/slack-ruby-bot-server/pull/60): Log caught Standard::Error backtrace at debug-level - [@alexagranov](https://github.com/alexagranov). * [#65](https://github.com/slack-ruby/slack-ruby-bot-server/pull/65): Updated Capybara and selenium-webdriver - [@dblock](https://github.com/dblock). * [#67](https://github.com/slack-ruby/slack-ruby-bot-server/pull/67): Only load the OTR::ActiveRecord::ConnectionManagement middleware when the OTR module is included. This module isn't needed when using Rails - [@darbyfrey](https://github.com/darbyfrey). +* [#74](https://github.com/slack-ruby/slack-ruby-bot-server/pull/74): Added ping worker, will restart offline bots - [@dblock](https://github.com/dblock). * Your contribution here. #### 0.6.1 (3/29/2017) diff --git a/README.md b/README.md index 97e2533..e8f8499 100644 --- a/README.md +++ b/README.md @@ -152,6 +152,10 @@ SlackRubyBotServer.configure do |config| end ``` +#### Ping Worker + +Each `SlackRubyBotServer::Server` instance will start a ping worker that will periodically check the online status of the bot and will forcefully close the connection if the bot goes offline, causing an automatic reconnect. + ### Access Tokens By default the implementation of [Team](lib/slack-ruby-bot-server/models/team) stores a `bot_access_token` that grants a certain amount of privileges to the bot user as described in [Slack OAuth Docs](https://api.slack.com/docs/oauth). You may not want a bot user at all, or may require different auth scopes, such as `users.profile:read` to access user profile information via `Slack::Web::Client#users_profile_get`. To obtain the non-bot access token make the following changes. diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index 30850de..301a983 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -57,7 +57,7 @@ def down! def restart! logger.warn "RESTART: #{owner}" - driver.emit(:close, WebSocket::Driver::CloseEvent.new(1001, 'bot offline')) + driver.emit(:close, WebSocket::Driver::CloseEvent.new(1001, 'bot offline')) if driver terminate end diff --git a/lib/slack-ruby-bot-server/version.rb b/lib/slack-ruby-bot-server/version.rb index 62d2232..2749c81 100644 --- a/lib/slack-ruby-bot-server/version.rb +++ b/lib/slack-ruby-bot-server/version.rb @@ -1,3 +1,3 @@ module SlackRubyBotServer - VERSION = '0.6.2'.freeze + VERSION = '0.7.0'.freeze end diff --git a/spec/slack-ruby-bot-server/ping_spec.rb b/spec/slack-ruby-bot-server/ping_spec.rb new file mode 100644 index 0000000..c218a14 --- /dev/null +++ b/spec/slack-ruby-bot-server/ping_spec.rb @@ -0,0 +1,101 @@ +require 'spec_helper' + +describe SlackRubyBotServer::Ping do + let(:team) { Team.new(token: 'token') } + let(:server) { SlackRubyBotServer::Server.new(team: team) } + let(:client) { server.send(:client) } + let(:options) { {} } + + subject do + SlackRubyBotServer::Ping.new(client, options) + end + + context 'with defaults' do + before do + allow(subject.wrapped_object).to receive(:every).and_yield + end + + it 'defaults retry count' do + expect(subject.send(:retry_count)).to eq 2 + end + + it 'calculates retries left' do + expect(subject.send(:retries_left)).to eq 2 + end + + it 'defaults ping interval' do + expect(subject.send(:ping_interval)).to eq 60 + end + + it 'checks for connection' do + expect(subject.wrapped_object).to receive(:check!) + subject.start! + end + + context 'after a failed check' do + before do + allow(subject.wrapped_object).to receive(:online?).and_return(false) + subject.start! + end + + it 'decrements retries left' do + expect(subject.send(:retries_left)).to eq 1 + end + + it 'sets error count' do + expect(subject.send(:error_count)).to eq 1 + end + + context 'after a successful check' do + before do + allow(subject.wrapped_object).to receive(:online?).and_return(true) + subject.start! + end + + it 're-increments retries left' do + expect(subject.send(:retries_left)).to eq 2 + end + + it 'resets error count' do + expect(subject.send(:error_count)).to eq 0 + end + end + end + + it 'terminates the ping worker after account_inactive' do + allow(subject.wrapped_object).to receive(:online?).and_raise('account_inactive') + expect(subject.wrapped_object).to receive(:terminate) + subject.start! + end + + it 'terminates after a number of retries' do + allow(subject.wrapped_object).to receive(:online?).and_return(false) + expect(subject.wrapped_object).to receive(:terminate) + 3.times { subject.start! } + end + end + + context 'with options' do + context 'ping interval' do + let(:options) { { ping_interval: 42 } } + + it 'is used' do + expect(subject.send(:ping_interval)).to eq 42 + expect(subject.wrapped_object).to receive(:every).with(42) + subject.start! + end + end + + context 'retry count' do + let(:options) { { retry_count: 42 } } + + it 'is set' do + expect(subject.send(:retry_count)).to eq 42 + end + + it 'adjusts retries left' do + expect(subject.send(:retries_left)).to eq 42 + end + end + end +end From 2c13bc6d0edfda6e51887e0861d4d2c96b7910d4 Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 21 Aug 2018 19:46:08 -0400 Subject: [PATCH 6/6] Added globally configurable ping options and a way to disable ping. --- .rubocop_todo.yml | 2 +- README.md | 14 ++++++++- UPGRADING.md | 18 +++++++++++ lib/slack-ruby-bot-server/config.rb | 2 ++ lib/slack-ruby-bot-server/server.rb | 11 ++++++- lib/slack-ruby-bot-server/service.rb | 4 ++- spec/slack-ruby-bot-server/ping_spec.rb | 36 +++++++++++++++++++--- spec/slack-ruby-bot-server/service_spec.rb | 15 +++++++++ 8 files changed, 93 insertions(+), 9 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 9b5a42f..ce78ff1 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2018-05-06 23:29:13 -0400 using RuboCop version 0.55.0. +# on 2018-08-21 19:45:36 -0400 using RuboCop version 0.55.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new diff --git a/README.md b/README.md index e8f8499..b20035e 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,19 @@ end #### Ping Worker -Each `SlackRubyBotServer::Server` instance will start a ping worker that will periodically check the online status of the bot and will forcefully close the connection if the bot goes offline, causing an automatic reconnect. +Each `SlackRubyBotServer::Server` instance will start a ping worker that will periodically check the online status of the bot via the Slack web [auth.test](https://api.slack.com/methods/auth.test) and [users.getPresence](https://api.slack.com/methods/users.getPresence) APIs, and will forcefully close the connection if the bot goes offline, causing an automatic reconnect. + +You can configure the ping interval, number of retries before restart, and disable the ping worker entirely. + +```ruby +SlackRubyBotServer.configure do |config| + config.ping = { + enabled: true, # set to false to disable the ping worker + ping_interval: 30, # interval in seconds + retry_count: 3 # number of unsuccessful retries until a restart + } +end +``` ### Access Tokens diff --git a/UPGRADING.md b/UPGRADING.md index 130dfc5..4ab8d98 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,24 @@ Upgrading Slack-Ruby-Bot-Server =============================== +### Upgrading to >= 0.7.0 + +#### New Ping Worker + +Version 0.7.0 will automatically start a ping worker that checks for the bot's online status and forcefully terminate and restart disconnected bots. Set the ping `enabled` option to `false` to disable this behavior. + +```ruby +SlackRubyBotServer.configure do |config| + config.ping = { + enabled: false + } +end +``` + +If you are currently using a custom ping worker as suggested in [slack-ruby-client#208](https://github.com/slack-ruby/slack-ruby-client/issues/208), delete it. + +See [#74](https://github.com/slack-ruby/slack-ruby-bot-server/pull/74) for more information. + ### Upgrading to >= 0.6.0 #### Mongoid and ActiveRecord support diff --git a/lib/slack-ruby-bot-server/config.rb b/lib/slack-ruby-bot-server/config.rb index 827b85e..85f487e 100644 --- a/lib/slack-ruby-bot-server/config.rb +++ b/lib/slack-ruby-bot-server/config.rb @@ -3,9 +3,11 @@ module Config extend self attr_accessor :server_class + attr_accessor :ping attr_accessor :database_adapter def reset! + self.ping = nil self.server_class = SlackRubyBotServer::Server self.database_adapter = if defined?(::Mongoid) :mongoid diff --git a/lib/slack-ruby-bot-server/server.rb b/lib/slack-ruby-bot-server/server.rb index 3c9c43a..bb02441 100644 --- a/lib/slack-ruby-bot-server/server.rb +++ b/lib/slack-ruby-bot-server/server.rb @@ -7,6 +7,7 @@ class Server < SlackRubyBot::Server def initialize(attrs = {}) attrs = attrs.dup @team = attrs.delete(:team) + @ping_options = attrs.delete(:ping) || {} raise 'Missing team' unless @team attrs[:token] = @team.token super(attrs) @@ -23,10 +24,18 @@ def restart!(wait = 1) private + attr_reader :ping_options + + def create_ping + return unless !ping_options.key?(:enabled) || ping_options[:enabled] + SlackRubyBotServer::Ping.new(client, ping_options) + end + def open! client.owner = team client.on :open do |_event| - SlackRubyBotServer::Ping.new(client).start! + worker = create_ping + worker.start! if worker end end end diff --git a/lib/slack-ruby-bot-server/service.rb b/lib/slack-ruby-bot-server/service.rb index bfb1d7c..9c3dbcb 100644 --- a/lib/slack-ruby-bot-server/service.rb +++ b/lib/slack-ruby-bot-server/service.rb @@ -30,7 +30,9 @@ def create!(team) def start!(team) run_callbacks :starting, team logger.info "Starting team #{team}." - server = SlackRubyBotServer::Config.server_class.new(team: team) + options = { team: team } + options[:ping] = SlackRubyBotServer::Config.ping if SlackRubyBotServer::Config.ping + server = SlackRubyBotServer::Config.server_class.new(options) start_server! team, server run_callbacks :started, team server diff --git a/spec/slack-ruby-bot-server/ping_spec.rb b/spec/slack-ruby-bot-server/ping_spec.rb index c218a14..d18c8ce 100644 --- a/spec/slack-ruby-bot-server/ping_spec.rb +++ b/spec/slack-ruby-bot-server/ping_spec.rb @@ -2,12 +2,12 @@ describe SlackRubyBotServer::Ping do let(:team) { Team.new(token: 'token') } - let(:server) { SlackRubyBotServer::Server.new(team: team) } - let(:client) { server.send(:client) } let(:options) { {} } + let(:server) { SlackRubyBotServer::Server.new({ team: team }.merge(options)) } + let(:client) { server.send(:client) } subject do - SlackRubyBotServer::Ping.new(client, options) + server.send(:create_ping) end context 'with defaults' do @@ -77,7 +77,7 @@ context 'with options' do context 'ping interval' do - let(:options) { { ping_interval: 42 } } + let(:options) { { ping: { ping_interval: 42 } } } it 'is used' do expect(subject.send(:ping_interval)).to eq 42 @@ -87,7 +87,7 @@ end context 'retry count' do - let(:options) { { retry_count: 42 } } + let(:options) { { ping: { retry_count: 42 } } } it 'is set' do expect(subject.send(:retry_count)).to eq 42 @@ -97,5 +97,31 @@ expect(subject.send(:retries_left)).to eq 42 end end + + context 'enabled' do + context 'nil' do + let(:options) { { ping: { enabled: nil } } } + + it 'does not create a worker' do + expect(subject).to be_nil + end + end + + context 'false' do + let(:options) { { ping: { enabled: false } } } + + it 'does not create a worker' do + expect(subject).to be_nil + end + end + + context 'true' do + let(:options) { { ping: { enabled: true } } } + + it 'creates a worker' do + expect(subject).to_not be_nil + end + end + end end end diff --git a/spec/slack-ruby-bot-server/service_spec.rb b/spec/slack-ruby-bot-server/service_spec.rb index 3f54da8..477a22c 100644 --- a/spec/slack-ruby-bot-server/service_spec.rb +++ b/spec/slack-ruby-bot-server/service_spec.rb @@ -59,6 +59,21 @@ def initialize(options = {}) SlackRubyBotServer::Service.instance.stop!(team) end end + context 'overriding ping' do + after do + SlackRubyBotServer.config.reset! + end + it 'creates an instance of server class' do + expect(SlackRubyBotServer::Server).to receive(:new).with(team: team, ping: { retry_interval: 42 }).and_call_original + allow_any_instance_of(SlackRubyBotServer::Server).to receive(:start_async) + allow_any_instance_of(SlackRubyBotServer::Server).to receive(:stop!) + SlackRubyBotServer.configure do |config| + config.ping = { retry_interval: 42 } + end + SlackRubyBotServer::Service.instance.start!(team) + SlackRubyBotServer::Service.instance.stop!(team) + end + end context 'callbacks' do before do @events = []