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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeout TTL set incorrectly #4

Closed
telkins opened this issue Feb 26, 2021 · 2 comments
Closed

Timeout TTL set incorrectly #4

telkins opened this issue Feb 26, 2021 · 2 comments

Comments

@telkins
Copy link

telkins commented Feb 26, 2021

I'm new to this package, so take what I say with a grain of salt. I could be wrong. 馃

I installed the latest/greatest today and tried it out a bit in the hopes of getting a better overall understanding of the leaky bucket and how it would look/feel for my particular use case.

Basically, I was finding that whenever I would seem to fill my bucket, it would be empty again...right away. 馃槼

It took a while of debugging/troubleshooting, which helped me learn a bit more of the ins/outs of the package and the concept in general, but I believe I found a problem.

There appears to be an issue in ArtisanSdk\RateLimiter\Limiter:

    public function timeout(int $duration = 1): void
    {
        if ($this->hasTimeout()) {
            return;
        }

        $this->cache->put(
            $this->getTimeoutKey(),
            (int) $this->lastBucket()->timer() + ($duration * 60),
            $duration
        );
    }

The problem appears to be in the call to put(). The ttl is never multiplied by 60, so it expires long before the timeout would expire. I believe $duration should be multiplied by 60 for the ttl as well.

I'll try to do a quick PR and relate it to this issue. I'll test it locally first, of course. 馃

Please let me know if you have any questions.

telkins added a commit to telkins/ratelimiter that referenced this issue Feb 26, 2021
When timing out, the ttl should be at least as long as the timeout.

Please see artisansdk#4
This was referenced Feb 26, 2021
telkins added a commit to telkins/ratelimiter that referenced this issue Feb 26, 2021
Related to artisansdk#4 , artisansdk#5 , and artisansdk#6 .

This one wasn't as straightforward for me as I thought it would be.  I initially thought to pass `$bucket->duration()` for the TTL, but it seemed like it would **only** increase.

So...my confidence is a little low when it comes to this proposed change.  I believe it's wrong, but I'm not sure that this is the best fix.  It does seem to work according to my expectations, though, fwiw.

On a bit of a negative note, I now wonder what's going on with duration.  Why would duration continue to increase?  And, if it shouldn't be increasing, then what is impacted by that?
@dalabarge
Copy link
Contributor

The $duration is set in minutes while it should/could be set in seconds. So line (int) $this->lastBucket()->timer() + ($duration * 60) becomes just (int) ($this->lastBucket()->timer() + $duration) as the cache will then work on Laravel 6+ as $duration will now be specified in seconds. Just sweep the code for references to timing and make sure they are all specified in seconds and that should fix things.

@dalabarge
Copy link
Contributor

Tag released 1.0.0-rc3 is compatible with specifying timeout() and duration() in seconds instead of minutes.

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

No branches or pull requests

2 participants