-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Bug fix in Throttler class check validation #2873
Conversation
@@ -139,8 +139,7 @@ public function check(string $key, int $capacity, int $seconds, int $cost = 1): | |||
$tokenName = $this->prefix . $key; | |||
|
|||
// Check to see if the bucket has even been created yet. | |||
if (($tokens = $this->cache->get($tokenName)) === null) | |||
{ | |||
if (($tokens = $this->cache->get($tokenName)) === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted to match CodeIgniters coding standard.
{ = should be on it's own line.
// we need to decrement the number of available tokens. | ||
if ($tokens > 0) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted to match CodeIgniters coding standard.
{ = should be on it's own line.
// we need to decrement the number of available tokens. | ||
if ($tokens > 0) | ||
{ | ||
if ($tokens >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about
if ($token)
I think I understand this, and it is a good move. @jlamim there are some good suggestions here, any chance you could take a pass at cleaning it up? |
It would be nice to have it fixed, but I think @jlamim is busy, so I'll merge it and then fix the code style myself. |
Description
While I was structuring an example to compose my book, I found that even if I correctly set the parameters for the
check()
method, a request was always executed more than the defined limit.This was because when checking for the existence of the cache (
($tokens = $this->cache->get($tokenName)) === null
) if it was null,true
was returned, and just created the cache, without start the count, which always allowed one more request, because only the next time the counter would start.The
return true
was removed in this check and in checking the amount of tokens it was adjusted to$tokens> = 0
.After these changes, the limit defined in
check
started to be applied successfully.Checklist: