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.

Sign up for GitHub

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

Jump to bottom

Fix timeout ttl #5

Closed
wants to merge 1 commit into from
Closed

Fix timeout ttl #5

wants to merge 1 commit into from

Conversation

telkins
Copy link

@telkins telkins commented Feb 26, 2021

When timing out, the ttl should be at least as long as the timeout.

Please see #4

@telkins
Fix timeout ttl
8651d60 
When timing out, the ttl should be at least as long as the timeout.

Please see artisansdk#4
@telkins
Copy link
Author

telkins commented Feb 26, 2021

OK. I'm not sure that it's related or not, but as I'm starting to submit another issue related to the TTL that's set when Limiter::hit() is called, I'm realizing that the code seems to be written such that TTL is in minutes and not seconds. I'm not sure if there are cache services that expect TTLs in minutes, but I'm under the impression that they're supposed to be in seconds.

I could be wrong, of course, but if I'm right, then there's a chance there are also other places in the code that I've not yet stumbled upon that may also need to be addressed.

Or...I'm wrong about the seconds/minutes stuff and we may have a bigger -- or at least different -- issue on our hands. 😅

@telkins telkins mentioned this pull request Feb 26, 2021
Closed
telkins added a commit to telkins/ratelimiter that referenced this pull request Feb 26, 2021
@telkins
Fix TTL on hit()...
c0dfe19 
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?
@telkins telkins mentioned this pull request Feb 26, 2021
Closed
@dalabarge
Copy link
Contributor

Thank you for the PR! Are you using Laravel 8 and 1.0.0-rc2 release? I just tagged that compatibility yesterday. This package was originally written for Laravel 5 but was ported for Laravel 8 and I'm thinking the problem is that Laravel switched from TTL in minutes to TTL in seconds. I'll look into which version of Laravel switched this parameter but it's probably got something to do with that.

@dalabarge
Copy link
Contributor

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

@dalabarge dalabarge closed this Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@telkins @dalabarge