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.

Sign up for GitHub

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

Jump to bottom

fix bug rate_limit_details returned and a bug for intercom request header #578

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix bug rate_limit_details returned and a bug for intercom request header #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa6ca10
fix bug rate_limit_details returned as nil if net:http failed a reque…
Goran1708 Dec 10, 2021
c123998
Merge branch 'master' into bugfix_rate_limit_details
johnnynader Mar 1, 2022
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
2 changes: 1 addition & 1 deletion lib/intercom/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def execute_request(request)
request.handle_rate_limit = handle_rate_limit
request.execute(@base_url, token: @token, api_version: @api_version, **timeouts)
ensure
@rate_limit_details = request.rate_limit_details
@rate_limit_details = request.rate_limit_details unless request.rate_limit_details.empty?
Copy link

Choose a reason for hiding this comment

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

Do you mean?

Suggested change
@rate_limit_details = request.rate_limit_details unless request.rate_limit_details.empty?
@rate_limit_details = request.rate_limit_details unless request.rate_limit_details.nil?

I think this is your intention from what I understand of the PR description

end

attr_writer :base_url
Expand Down
4 changes: 2 additions & 2 deletions lib/intercom/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,11 @@ def execute(target_base_url = nil, token:, read_timeout: 90, open_timeout: 30, a
parsed_body
rescue Intercom::RateLimitExceeded => e
if @handle_rate_limit
seconds_to_retry = (@rate_limit_details[:reset_at] - Time.now.utc).ceil
seconds_to_retry = ((@rate_limit_details[:reset_at] || Time.now.utc) - Time.now.utc).ceil
Copy link

Choose a reason for hiding this comment

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

If I understand correctly, this line is basically implementing a default seconds_to_retry value in the case that @rate_limit_details[:reset_at] is nil. Would it make more sense for this default value to be >0? If we are hitting a rate limit, it seems to me like the default behaviour should be to wait for a bit before a retry.

Copy link
Author

Choose a reason for hiding this comment

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

if there is a problem with the intercom server, or something like that, where this is not getting reset_at, we don't want to keep sleeping the application because we don't know if we are hitting the rate limit. If we hit the rate limit, the error will be raised where we will try to redo the call. If we hit it multiple times it will just "crash" and we will get a response error in the end user. But yeah...maybe I should convert Time.now.utc into 0. Not needed to use that "expensive" solution.

if (retries -= 1) < 0
raise Intercom::RateLimitExceeded, 'Rate limit retries exceeded. Please examine current API Usage.'
else
sleep seconds_to_retry unless seconds_to_retry < 0
sleep seconds_to_retry unless seconds_to_retry <= 0
Copy link

@samrjenkins samrjenkins Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps not worth changing this behaviour in this PR to keep the changes in this branch as atomic as possible. All we need to do is protect against an ArgumentError in the case of negative arguments.
That being said, a more "Ruby" implementation might be

Suggested change
sleep seconds_to_retry unless seconds_to_retry <= 0
sleep seconds_to_retry unless seconds_to_retry.negative?

Copy link
Author

Choose a reason for hiding this comment

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

.negative? is (-1, -2, -3), we want to include (0, -1, -2, -3). :o

Copy link

Choose a reason for hiding this comment

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

My thinking was that the current <= implementation is just there to prevent an ArgumentError when we call sleep with a negative argument. Do we really want to protect against a 0 argument here?

My feeling is that changing this condition is not really relevant to the change we are looking to make in this PR so we might be best leaving this condition unchanged.

Copy link
Author

Choose a reason for hiding this comment

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

Well if possible would optimise it a bit. I think if you want to sleep something for 0 seconds...its kinda off.

Copy link

@samrjenkins samrjenkins Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you're keen to make this change then perhaps this?

Suggested change
sleep seconds_to_retry unless seconds_to_retry <= 0
sleep seconds_to_retry if seconds_to_retry.positive?

retry
end
else
Expand Down
8 changes: 8 additions & 0 deletions spec/unit/intercom/client_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ module Intercom

client.get('/contacts', id: '123')
end

it 'sets rate limit details to empty hash' do
stub_request(:any, "https://api.intercom.io/test").to_raise(StandardError)

expect { client.get('/test', {}) }.must_raise(StandardError)

client.rate_limit_details.must_equal({})
end
end

describe 'OAuth clients' do
Expand Down
11 changes: 11 additions & 0 deletions spec/unit/intercom/request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ def execute!
execute!
end

it 'should not call sleep for rate limit if reset is not received' do
stub_request(:any, uri).to_return(
status: [429, "Too Many Requests"],
).then.to_return(status: [200, "OK"], body: default_body)

req.handle_rate_limit=true
req.expects(:sleep).never.with(any_parameters)

execute!
end

it 'should not sleep if rate limit reset time has passed' do
stub_request(:any, uri).to_return(
status: [429, "Too Many Requests"],
Expand Down