From d1aa8a43edb72910feb813fa0eb7bc9fabaf803a Mon Sep 17 00:00:00 2001 From: dblock Date: Mon, 27 Aug 2018 14:37:37 +0200 Subject: [PATCH 1/5] Replace celluloid-io with async-websocket. --- .rubocop_todo.yml | 2 +- CHANGELOG.md | 3 ++- Gemfile | 3 +++ UPGRADING.md | 8 +++++++ lib/slack-ruby-bot-server.rb | 2 +- lib/slack-ruby-bot-server/ping.rb | 19 ++++++++------- lib/slack-ruby-bot-server/version.rb | 2 +- sample_apps/sample_app_activerecord/Gemfile | 1 + sample_apps/sample_app_activerecord/config.ru | 6 ----- sample_apps/sample_app_mongoid/Gemfile | 1 + sample_apps/sample_app_mongoid/config.ru | 6 ----- slack-ruby-bot-server.gemspec | 2 +- spec/slack-ruby-bot-server/ping_spec.rb | 24 +++++++++---------- 13 files changed, 42 insertions(+), 37 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index c7f944e..09143f8 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2018-08-22 08:29:35 -0400 using RuboCop version 0.58.2. +# on 2018-08-27 14:23:13 +0200 using RuboCop version 0.58.2. # 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/CHANGELOG.md b/CHANGELOG.md index 0d87faf..278db33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,8 @@ ### Changelog -#### 0.7.1 (Next) +#### 0.8.0 (Next) +* [#75](https://github.com/slack-ruby/slack-ruby-bot-server/pull/75): Default to `async-websocket` instead of `celluloid-io` - [@dblock](https://github.com/dblock). * Your contribution here. #### 0.7.0 (8/22/2018) diff --git a/Gemfile b/Gemfile index 48d27c5..b760087 100644 --- a/Gemfile +++ b/Gemfile @@ -16,6 +16,9 @@ else warn "Invalid ENV['DATABASE_ADAPTER']: #{ENV['DATABASE_ADAPTER']}." end +gem 'slack-ruby-bot', github: 'dblock/slack-ruby-bot', branch: 'async' +gem 'slack-ruby-client', github: 'dblock/slack-ruby-client', branch: 'async' + gemspec group :development, :test do diff --git a/UPGRADING.md b/UPGRADING.md index 4ab8d98..91ff05e 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,14 @@ Upgrading Slack-Ruby-Bot-Server =============================== +### Upgrading to >= 0.8.0 + +### Different Asynchronous I/O Library + +The library now uses [async-websocket](https://github.com/socketry/async-websocket) instead of [celluloid-io](https://github.com/celluloid/celluloid-io). If your application is built on Celluloid you may need to make changes and use `Async::Reactor.run` and the likes. + +See [#75](https://github.com/slack-ruby/slack-ruby-bot-server/pull/75) for more information. + ### Upgrading to >= 0.7.0 #### New Ping Worker diff --git a/lib/slack-ruby-bot-server.rb b/lib/slack-ruby-bot-server.rb index d76600d..3c4ea28 100644 --- a/lib/slack-ruby-bot-server.rb +++ b/lib/slack-ruby-bot-server.rb @@ -1,4 +1,4 @@ -require 'celluloid/current' +require 'async/websocket' require 'grape-swagger' require 'slack-ruby-bot' diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index 301a983..48882db 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -1,7 +1,5 @@ module SlackRubyBotServer class Ping - include Celluloid - attr_reader :client attr_reader :options attr_reader :error_count @@ -13,8 +11,11 @@ def initialize(client, options = {}) end def start! - every ping_interval do - check! + ::Async::Reactor.run do |task| + loop do + task.sleep ping_interval + break unless check! + end end end @@ -23,6 +24,7 @@ def start! def check! if online? @error_count = 0 + true else down! end @@ -30,9 +32,10 @@ def check! case e.message when 'account_inactive', 'invalid_auth' then logger.warn "Error pinging team #{owner.id}: #{e.message}, terminating." - terminate + false else logger.warn "Error pinging team #{owner.id}: #{e.message}." + true end end @@ -51,14 +54,14 @@ def presence def down! logger.warn "DOWN: #{owner}, #{retries_left} #{retries_left == 1 ? 'retry' : 'retries'} left" @error_count += 1 - return if retries_left? + return true if retries_left? restart! + false end def restart! logger.warn "RESTART: #{owner}" - driver.emit(:close, WebSocket::Driver::CloseEvent.new(1001, 'bot offline')) if driver - terminate + driver.close if driver end def ping_interval diff --git a/lib/slack-ruby-bot-server/version.rb b/lib/slack-ruby-bot-server/version.rb index e6a71c6..b0d1805 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.7.1'.freeze + VERSION = '0.8.0'.freeze end diff --git a/sample_apps/sample_app_activerecord/Gemfile b/sample_apps/sample_app_activerecord/Gemfile index 5b2d79f..34d2a89 100644 --- a/sample_apps/sample_app_activerecord/Gemfile +++ b/sample_apps/sample_app_activerecord/Gemfile @@ -7,6 +7,7 @@ gem 'pg' gem 'rack-server-pages' gem 'rack-test' gem 'slack-ruby-bot-server', path: '../../' +gem 'slack-ruby-client', github: 'dblock/slack-ruby-client', branch: 'async' group :development, :test do gem 'standalone_migrations', '~> 5.2' diff --git a/sample_apps/sample_app_activerecord/config.ru b/sample_apps/sample_app_activerecord/config.ru index dfe10ec..af00a17 100644 --- a/sample_apps/sample_app_activerecord/config.ru +++ b/sample_apps/sample_app_activerecord/config.ru @@ -7,12 +7,6 @@ require 'yaml' ActiveRecord::Base.establish_connection(YAML.load_file('config/postgresql.yml')[ENV['RACK_ENV']]) -if ENV['RACK_ENV'] == 'development' - puts 'Loading NewRelic in developer mode ...' - require 'new_relic/rack/developer_mode' - use NewRelic::Rack::DeveloperMode -end - NewRelic::Agent.manual_start SlackRubyBotServer::App.instance.prepare! diff --git a/sample_apps/sample_app_mongoid/Gemfile b/sample_apps/sample_app_mongoid/Gemfile index 320880d..4526a89 100644 --- a/sample_apps/sample_app_mongoid/Gemfile +++ b/sample_apps/sample_app_mongoid/Gemfile @@ -3,6 +3,7 @@ source 'https://rubygems.org' gem 'mongoid' gem 'newrelic-slack-ruby-bot' gem 'slack-ruby-bot-server', path: '../../' +gem 'slack-ruby-client', github: 'dblock/slack-ruby-client', branch: 'async' gem 'kaminari-mongoid' gem 'mongoid-scroll' diff --git a/sample_apps/sample_app_mongoid/config.ru b/sample_apps/sample_app_mongoid/config.ru index 50bb7ad..af0d0a5 100644 --- a/sample_apps/sample_app_mongoid/config.ru +++ b/sample_apps/sample_app_mongoid/config.ru @@ -6,12 +6,6 @@ require_relative 'commands' Mongoid.load!(File.expand_path('config/mongoid.yml', __dir__), ENV['RACK_ENV']) -if ENV['RACK_ENV'] == 'development' - puts 'Loading NewRelic in developer mode ...' - require 'new_relic/rack/developer_mode' - use NewRelic::Rack::DeveloperMode -end - NewRelic::Agent.manual_start SlackRubyBotServer::App.instance.prepare! diff --git a/slack-ruby-bot-server.gemspec b/slack-ruby-bot-server.gemspec index 72eaefa..684f1fe 100644 --- a/slack-ruby-bot-server.gemspec +++ b/slack-ruby-bot-server.gemspec @@ -14,7 +14,7 @@ Gem::Specification.new do |spec| spec.files = `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(spec)/}) } spec.require_paths = ['lib'] - spec.add_dependency 'celluloid-io' + spec.add_dependency 'async-websocket' spec.add_dependency 'foreman' spec.add_dependency 'grape' spec.add_dependency 'grape-roar', '>= 0.4.0' diff --git a/spec/slack-ruby-bot-server/ping_spec.rb b/spec/slack-ruby-bot-server/ping_spec.rb index d18c8ce..9f84046 100644 --- a/spec/slack-ruby-bot-server/ping_spec.rb +++ b/spec/slack-ruby-bot-server/ping_spec.rb @@ -12,7 +12,7 @@ context 'with defaults' do before do - allow(subject.wrapped_object).to receive(:every).and_yield + allow_any_instance_of(Async::Task).to receive(:sleep) end it 'defaults retry count' do @@ -28,14 +28,14 @@ end it 'checks for connection' do - expect(subject.wrapped_object).to receive(:check!) + expect(subject).to receive(:check!).and_return(false) subject.start! end context 'after a failed check' do before do - allow(subject.wrapped_object).to receive(:online?).and_return(false) - subject.start! + allow(subject).to receive(:online?).and_return(false) + subject.send(:check!) end it 'decrements retries left' do @@ -48,8 +48,8 @@ context 'after a successful check' do before do - allow(subject.wrapped_object).to receive(:online?).and_return(true) - subject.start! + allow(subject).to receive(:online?).and_return(true) + subject.send(:check!) end it 're-increments retries left' do @@ -63,15 +63,14 @@ 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) + allow(subject).to receive(:online?).and_raise('account_inactive') 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! } + allow(subject).to receive(:online?).and_return(false) + expect(subject).to receive(:check!).exactly(3).times.and_call_original + subject.start! end end @@ -80,8 +79,9 @@ let(:options) { { ping: { ping_interval: 42 } } } it 'is used' do + expect_any_instance_of(Async::Task).to receive(:sleep).with(42) expect(subject.send(:ping_interval)).to eq 42 - expect(subject.wrapped_object).to receive(:every).with(42) + expect(subject).to receive(:check!).and_return(false) subject.start! end end From 308ffb2d55fb205c06047d9a487a7caedc24d561 Mon Sep 17 00:00:00 2001 From: dblock Date: Wed, 29 Aug 2018 15:10:56 +0200 Subject: [PATCH 2/5] Added debug logging to ping thread. --- lib/slack-ruby-bot-server/ping.rb | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index 48882db..e1737bb 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -12,15 +12,24 @@ def initialize(client, options = {}) def start! ::Async::Reactor.run do |task| - loop do - task.sleep ping_interval - break unless check! - end + run(task) end end private + def run(task) + logger.debug "PING: #{owner}, every #{ping_interval} second(s)" + loop do + task.sleep ping_interval + break unless check! + end + logger.debug "PING: #{owner}, done." + rescue StandardError => e + logger.error e + raise e + end + def check! if online? @error_count = 0 @@ -62,6 +71,8 @@ def down! def restart! logger.warn "RESTART: #{owner}" driver.close if driver + rescue StandardError => e + logger.warn "Error restarting team #{owner.id}: #{e.message}." end def ping_interval From f74bb9c0160d656b135f9f9b61b1a8720e78771d Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 4 Sep 2018 11:38:58 -0400 Subject: [PATCH 3/5] Truncate error message from ping thread. --- lib/slack-ruby-bot-server/ping.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index e1737bb..1db8b5c 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -1,3 +1,5 @@ +require 'active_support/core_ext/string/filters' + module SlackRubyBotServer class Ping attr_reader :client @@ -43,7 +45,8 @@ def check! logger.warn "Error pinging team #{owner.id}: #{e.message}, terminating." false else - logger.warn "Error pinging team #{owner.id}: #{e.message}." + message = e.message.truncate(24, separator: "\n", omission: '...') + logger.warn "Error pinging team #{owner.id}: #{message}." true end end @@ -69,7 +72,7 @@ def down! end def restart! - logger.warn "RESTART: #{owner}" + logger.warn "RESTART: #{owner}, #{driver}" driver.close if driver rescue StandardError => e logger.warn "Error restarting team #{owner.id}: #{e.message}." From b9028462c2a6fc5fb158682bea750c4768bdeb66 Mon Sep 17 00:00:00 2001 From: dblock Date: Wed, 5 Sep 2018 20:15:06 -0400 Subject: [PATCH 4/5] Close socket, not driver. --- .rubocop_todo.yml | 7 ++++++- Gemfile | 3 --- lib/slack-ruby-bot-server/ping.rb | 14 ++++++++++++-- sample_apps/sample_app_activerecord/Gemfile | 1 - sample_apps/sample_app_mongoid/Gemfile | 1 - 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 09143f8..fbcddf4 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2018-08-27 14:23:13 +0200 using RuboCop version 0.58.2. +# on 2018-09-08 18:09:56 -0400 using RuboCop version 0.58.2. # 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 @@ -16,6 +16,11 @@ Layout/IndentHeredoc: - 'sample_apps/sample_app_activerecord/commands/help.rb' - 'sample_apps/sample_app_mongoid/commands/help.rb' +# Offense count: 1 +Lint/HandleExceptions: + Exclude: + - 'lib/slack-ruby-bot-server/ping.rb' + # Offense count: 3 # Configuration parameters: Blacklist. # Blacklist: (?-mix:(^|\s)(EO[A-Z]{1}|END)(\s|$)) diff --git a/Gemfile b/Gemfile index b760087..48d27c5 100644 --- a/Gemfile +++ b/Gemfile @@ -16,9 +16,6 @@ else warn "Invalid ENV['DATABASE_ADAPTER']: #{ENV['DATABASE_ADAPTER']}." end -gem 'slack-ruby-bot', github: 'dblock/slack-ruby-bot', branch: 'async' -gem 'slack-ruby-client', github: 'dblock/slack-ruby-client', branch: 'async' - gemspec group :development, :test do diff --git a/lib/slack-ruby-bot-server/ping.rb b/lib/slack-ruby-bot-server/ping.rb index 1db8b5c..272b7b8 100644 --- a/lib/slack-ruby-bot-server/ping.rb +++ b/lib/slack-ruby-bot-server/ping.rb @@ -72,8 +72,14 @@ def down! end def restart! - logger.warn "RESTART: #{owner}, #{driver}" - driver.close if driver + logger.warn "RESTART: #{owner}" + begin + connection.close + rescue Async::Wrapper::Cancelled + # ignore, from connection.close + end + driver.close + driver.emit(:close, WebSocket::Driver::CloseEvent.new(1001, 'bot offline')) rescue StandardError => e logger.warn "Error restarting team #{owner.id}: #{e.message}." end @@ -102,6 +108,10 @@ def driver socket.instance_variable_get(:@driver) if socket end + def connection + driver.instance_variable_get(:@socket) if driver + end + def logger @logger ||= begin STDOUT.sync = true diff --git a/sample_apps/sample_app_activerecord/Gemfile b/sample_apps/sample_app_activerecord/Gemfile index 34d2a89..5b2d79f 100644 --- a/sample_apps/sample_app_activerecord/Gemfile +++ b/sample_apps/sample_app_activerecord/Gemfile @@ -7,7 +7,6 @@ gem 'pg' gem 'rack-server-pages' gem 'rack-test' gem 'slack-ruby-bot-server', path: '../../' -gem 'slack-ruby-client', github: 'dblock/slack-ruby-client', branch: 'async' group :development, :test do gem 'standalone_migrations', '~> 5.2' diff --git a/sample_apps/sample_app_mongoid/Gemfile b/sample_apps/sample_app_mongoid/Gemfile index 4526a89..320880d 100644 --- a/sample_apps/sample_app_mongoid/Gemfile +++ b/sample_apps/sample_app_mongoid/Gemfile @@ -3,7 +3,6 @@ source 'https://rubygems.org' gem 'mongoid' gem 'newrelic-slack-ruby-bot' gem 'slack-ruby-bot-server', path: '../../' -gem 'slack-ruby-client', github: 'dblock/slack-ruby-client', branch: 'async' gem 'kaminari-mongoid' gem 'mongoid-scroll' From 3039fd3ea1032401fbda644168c9845ab7ee8487 Mon Sep 17 00:00:00 2001 From: dblock Date: Sat, 8 Sep 2018 18:08:28 -0400 Subject: [PATCH 5/5] Adjust wait time as per slack hint on Slack::Web::Api::Errors::TooManyRequestsError. --- CHANGELOG.md | 1 + lib/slack-ruby-bot-server/service.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 278db33..123910b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ #### 0.8.0 (Next) * [#75](https://github.com/slack-ruby/slack-ruby-bot-server/pull/75): Default to `async-websocket` instead of `celluloid-io` - [@dblock](https://github.com/dblock). +* [#76](https://github.com/slack-ruby/slack-ruby-bot-server/pull/76): Adjust wait time on restart on Slack::Web::Api::Errors::TooManyRequestsError - [@dblock](https://github.com/dblock). * Your contribution here. #### 0.7.0 (8/22/2018) diff --git a/lib/slack-ruby-bot-server/service.rb b/lib/slack-ruby-bot-server/service.rb index 9c3dbcb..4c94727 100644 --- a/lib/slack-ruby-bot-server/service.rb +++ b/lib/slack-ruby-bot-server/service.rb @@ -97,6 +97,7 @@ def start_server!(team, server, wait = 1) logger.error "#{team.name}: #{e.message}, team will be deactivated." deactivate!(team) else + wait = e.retry_after if e.is_a?(Slack::Web::Api::Errors::TooManyRequestsError) logger.error "#{team.name}: #{e.message}, restarting in #{wait} second(s)." sleep(wait) start_server! team, server, [wait * 2, 60].min