Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust wait time as per slack hint on Slack::Web::Api::Errors::TooManyRequestsError #76

Merged
merged 5 commits into from
Sep 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -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-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
Expand All @@ -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|$))
Expand Down
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
### 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).
* [#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)
Expand Down
8 changes: 8 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/slack-ruby-bot-server.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require 'celluloid/current'
require 'async/websocket'

require 'grape-swagger'
require 'slack-ruby-bot'
Expand Down
45 changes: 36 additions & 9 deletions lib/slack-ruby-bot-server/ping.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'active_support/core_ext/string/filters'

module SlackRubyBotServer
class Ping
include Celluloid

attr_reader :client
attr_reader :options
attr_reader :error_count
Expand All @@ -13,26 +13,41 @@ def initialize(client, options = {})
end

def start!
every ping_interval do
check!
::Async::Reactor.run do |task|
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
true
else
down!
end
rescue StandardError => e
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}."
message = e.message.truncate(24, separator: "\n", omission: '...')
logger.warn "Error pinging team #{owner.id}: #{message}."
true
end
end

Expand All @@ -51,14 +66,22 @@ 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
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

def ping_interval
Expand All @@ -85,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
Expand Down
1 change: 1 addition & 0 deletions lib/slack-ruby-bot-server/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/slack-ruby-bot-server/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module SlackRubyBotServer
VERSION = '0.7.1'.freeze
VERSION = '0.8.0'.freeze
end
6 changes: 0 additions & 6 deletions sample_apps/sample_app_activerecord/config.ru
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
6 changes: 0 additions & 6 deletions sample_apps/sample_app_mongoid/config.ru
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
2 changes: 1 addition & 1 deletion slack-ruby-bot-server.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
24 changes: 12 additions & 12 deletions spec/slack-ruby-bot-server/ping_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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

Expand All @@ -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
Expand Down