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

sleep on requests limit reach issue #15

Open
Art3miX opened this issue Jun 11, 2018 · 5 comments
Open

sleep on requests limit reach issue #15

Art3miX opened this issue Jun 11, 2018 · 5 comments

Comments

@Art3miX
Copy link

Art3miX commented Jun 11, 2018

Hey, trying to understand why you did it this way, i didn't test it yet, but it doesn't seems right

  1. if for example the call limit your app can make is 40, the callLimitReached function will return true if you still have 8 calls left,
  2. you are doing a random sleep of between 3-10 seconds if the "limit" is reached

It seems like worse case you will wait 10 seconds to make a request while you still have 8 requests available left, best case is 3 seconds.

You are doing the limit check after each request, so why don't you just check if you have 1 available request left, if you do sleep for 1 second?

Anything i'm missing why this was built this way?

@baorv
Copy link

baorv commented Aug 29, 2018

I totally agree with to you. The api limit check is still not optimized

@baorv
Copy link

baorv commented Aug 29, 2018

Do you have any idea to deal with this problem? @Art3miX

@Art3miX
Copy link
Author

Art3miX commented Aug 29, 2018

I didn't use this lib in the end and can't do a pull request.

But yea, its pretty easy to change it.

in client.php :
getCallLimit should return the exact number of request left, so we take the bucket size and reduce the amount of calls we already did :
return $this->call_bucket - $this->call_limit;

callLimitReached should return true if 1 or less requests left (just in case we leave extra 1 request in bucket).
return $this->getCallLimit() <= 1;

And the sleep should be changed to 1 second, because shopify give us 2 requests each second, so we wait 1 second and we get extra 2 requests, this will ensure us we only wait the minimum time needed between requests.
in request function:

if ($this->rate_limit && self::$delay_next_call) {
      // Sleep a random amount of time to help prevent bucket overflow.
      usleep(1 * 1000000);
    }

Sorry for this poor answer, but this should get you going (unless i forgot something), i'm sure someone can find the time to do a pull request.

@baorv
Copy link

baorv commented Aug 29, 2018

Ok. I think it's great idea. But how can I do it inside this lib without processing in the request method?

@baorv
Copy link

baorv commented Aug 29, 2018

I found a solve in the internet by reading header named Retry-After, but I don't test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants