rdohms / meetup-api-client

Guzzle powered Meetup.com API Client
MIT License
76 stars 24 forks source link

Warning: Division by zero in src/DMS/Service/Meetup/Plugin/RateLimitPlugin.php on line 148 #53

Closed DragonBe closed 6 years ago

DragonBe commented 6 years ago

When iterating over meetup groups and in each iteration fetching details of the user group leader, I hit this error.

Warning: Division by zero in src/DMS/Service/Meetup/Plugin/RateLimitPlugin.php on line 148
DragonBe commented 6 years ago

Consequently the following error is triggered, which indicates the values aren't validated before passing on.

Warning: usleep() expects parameter 1 to be integer, float given in src/DMS/Service/Meetup/Plugin/RateLimitPlugin.php on line 149
rdohms commented 6 years ago

Interesting, reaching zero is supposed to not be possible, since the sleep call is always trying to make sure you don't make calls that don't fit.

But indeed if you do somehow reach it, then the error is there.

I'm wondering what the proper behaviour in this case is, cause i believe if you have no more requests, you should sleep until it resets, not sleep for zero seconds which would only get you a rate limited request.

I have rewritten the patch in #55 but i want to confirm with @dennisdegreef if that would be the expected behaviour.

dennisdegreef commented 6 years ago

The intent was trying to spread out requests so it wouldn't cause 'a lot of requests' followed by 'X amount of seconds sleeping'. Since the rate limit is a global state and this was tested single threaded, I can imagine this causing problems otherwise.

I agree that the values should be (and could have been better) validated.

Maybe we also need to make the single/multiple process impact more clearly in the documentation, because this solution will still cause some requests to sleep for a relatively long time after the rate limit has been hit.

Even more, all sequential requests will sleep the amount of seconds remaining, until they all start executing again, which will exhaust the fresh rate limit directly, causing the same behaviour repeatedly. This is, if the amount of requests are fired in a constant time instead of bursts (which is the case with async iterations over vast amount of data).

If this makes sense :P

rdohms commented 6 years ago

Fixed by #55