fix bug rate_limit_details returned and a bug for intercom request header #578
Conversation
…st, and bug if intercom request header does not contain X-RateLimit-Reset header
| 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 |
There was a problem hiding this comment.
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
| sleep seconds_to_retry unless seconds_to_retry <= 0 | |
| sleep seconds_to_retry unless seconds_to_retry.negative? |
There was a problem hiding this comment.
.negative? is (-1, -2, -3), we want to include (0, -1, -2, -3). :o
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well if possible would optimise it a bit. I think if you want to sleep something for 0 seconds...its kinda off.
There was a problem hiding this comment.
If you're keen to make this change then perhaps this?
| sleep seconds_to_retry unless seconds_to_retry <= 0 | |
| sleep seconds_to_retry if seconds_to_retry.positive? |
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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? | |||
There was a problem hiding this comment.
Do you mean?
| @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
|
Hey @Goran1708 Wondering if this pr is still in progress or if you'd like us to give it a review |
Heya! Yeah sure, you can give it a review. Sorry I missed your notification. |
Fix bug rate_limit_details returned as nil if net:http failed a request, and bug if intercom request header does not contain X-RateLimit-Reset header
Why?
Why are you making this change?
We have experienced a "bug" when using this library, we implemented our own sidekiq throttle limiter with existing
rate_limit_details. It worked fine until at one point(my guess is) NET:HTTP request started failing andrate_limit_detailswas being returned as nil.Another potential issue is if intercom does not return
X-RateLimit-Resetheader, the app will break because it will try to calculate try to subtract some time from Nil.How?
Technical details on your change
For rate_limit_details:
request.rate_limit_detailsif its nil, because it should be empty hash in the initialiser.rate_limit_detailsto request object. It's kind of an anti-pattern that both classes are initialising the same variable and both of them are using it. We should initialise it in Client, pass it to Request, Request can modify it and send it back if needed.For bug prone date arithmetic: