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

respect rate limits during pagination by sleeping #167

Merged
merged 9 commits into from Sep 19, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
@@ -1,5 +1,6 @@
### 0.9.2 (Next)

* [#167](https://github.com/slack-ruby/slack-ruby-client/pull/167): Respect rate limits during pagination by sleeping. Also add optional `pause` parameter in order to proactively sleep between each paginated request - [@jmanian](https://github.com/jmanian).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe simply "Added support for pausing between paginated requests that cause Slack rate limiting"?

* [#163](https://github.com/slack-ruby/slack-ruby-client/pull/164): Use `OpenSSL::X509::DEFAULT_CERT_DIR` and `OpenSSL::X509::DEFAULT_CERT_FILE` for default ca_cert and ca_file - [@leifcr](https://github.com/leifcr).
* [#161](https://github.com/slack-ruby/slack-ruby-client/pull/161): Added support for cursor pagination - [@dblock](https://github.com/dblock).
* Your contribution here.
Expand Down
10 changes: 9 additions & 1 deletion lib/slack/web/pagination/cursor.rb
Expand Up @@ -7,23 +7,31 @@ class Cursor

attr_reader :client
attr_reader :verb
attr_reader :pause
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a bit uncomfortable calling this pause because it's unclear whether that's a verb or a noun, maybe sleep_interval? Don't feel strongly about this one though.

attr_reader :params

def initialize(client, verb, params = {})
@client = client
@verb = verb
@pause = params.delete(:pause)
@params = params
end

def each
next_cursor = nil
loop do
query = { limit: client.default_page_size }.merge(params).merge(cursor: next_cursor)
response = client.send(verb, query)
begin
response = client.send(verb, query)
rescue Slack::Web::Api::Errors::TooManyRequestsError => e
sleep(e.retry_after.seconds)
next
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think we should have a max retry count with some default set fairly high. If slack blacklists you somehow you'll have code that never ends and it will be impossible to debug.

I also think there should be a log line here in DEBUG mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think the count should reset after a successful request?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so.

end
yield response
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can move this inside the rescue block and avoid next, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also did I tell you that I was a nitpicky code reviewer? Hang on tight :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, I'm the same way 😎.

Which part are you suggesting go in the rescue block? I started out with everything inside the begin block (see first commit b75bfcb) which avoids the next. Is this what you mean?

I changed it (8542d00) because I thought it was bad practice to have more lines than necessary inside the begin block — even if just for clarity (so that it's clear which line is expected to raise the error). But in this case it's obvious where the error is coming from, so I guess it's fine either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually now think yours is better. Someone can do something inside the yield that causes that exception and we're going to have bad side effects and catching an error we shouldn't.

break unless response.response_metadata
next_cursor = response.response_metadata.next_cursor
break if next_cursor.blank?
sleep(pause) if pause
end
end
end
Expand Down
24 changes: 24 additions & 0 deletions spec/slack/web/api/pagination/cursor_spec.rb
Expand Up @@ -21,8 +21,20 @@
Slack::Messages::Message.new(response_metadata: { next_cursor: 'next' }),
Slack::Messages::Message.new
)
expect(cursor).not_to receive(:sleep)
cursor.to_a
end
context 'with rate limiting' do
let(:error) { Slack::Web::Api::Errors::TooManyRequestsError.new(nil) }
it 'sleeps after a TooManyRequestsError' do
expect(client).to receive(:users_list).with(limit: 100, cursor: nil).ordered.and_return(Slack::Messages::Message.new(response_metadata: { next_cursor: 'next' }))
expect(client).to receive(:users_list).with(limit: 100, cursor: 'next').ordered.and_raise(error)
expect(error).to receive(:retry_after).once.ordered.and_return(9)
expect(cursor).to receive(:sleep).once.ordered.with(9)
expect(client).to receive(:users_list).with(limit: 100, cursor: 'next').ordered.and_return(Slack::Messages::Message.new)
cursor.to_a
end
end
end
context 'with a custom limit' do
let(:cursor) { Slack::Web::Api::Pagination::Cursor.new(client, 'users_list', limit: 42) }
Expand All @@ -31,4 +43,16 @@
cursor.first
end
end
context 'with a custom pause' do
let(:cursor) { Slack::Web::Api::Pagination::Cursor.new(client, 'users_list', pause: 3) }
it 'sleeps between requests' do
expect(client).to receive(:users_list).exactly(3).times.and_return(
Slack::Messages::Message.new(response_metadata: { next_cursor: 'next_a' }),
Slack::Messages::Message.new(response_metadata: { next_cursor: 'next_b' }),
Slack::Messages::Message.new
)
expect(cursor).to receive(:sleep).with(3).twice
cursor.to_a
end
end
end