-
Notifications
You must be signed in to change notification settings - Fork 194
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
apcu_inc/apcu_dec increments and returns value even if the key is expired #183
Comments
After much hair pulling today I found this bug causing issues in our production rate limiter on APCu 4.0.7 / PHP 5.6.20-0+deb8u1 |
I just ran into this same issue on Ubuntu 16.04.
System info:
Edit: I worked around this with the following for now:
|
I ran into the same issue: PHP 7:
In PHP 5.6 this works fine:
To test this we have a PHP 7.0 and a PHP 5.6 docker container with APCu enabled:
Then run these commands:
I now also added this as workaround but having this solved in a consistent way across multiple PHP versions would be nice.
|
Fix an issue with APCus inc and dec methods on PHP 7 see krakjoe/apcu#183 (comment) for details
This was a purposeful change that made sense when I reviewed it. I'll have to think about that ... |
Fix an issue with APCus inc and dec methods on PHP 7 see krakjoe/apcu#183 (comment) for details Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Fix an issue with APCus inc and dec methods on PHP 7 see krakjoe/apcu#183 (comment) for details Signed-off-by: Morris Jobke <hey@morrisjobke.de>
I've implemented two changes to address this:
The original report also suggest to...
Does this sound sensible? I'm not super happy about the situation here, but I think this will at least make things make sense going forward. |
Just to clarify: Will the original TTL be preserved on inc/dec in case the record DIDNT expired so we are incrementing and NOT resetting over? And is this TTL since last increment or since when the record was stored? |
@fabicz If the record did not expire, the original TTL is preserved. The TTL is since the time of creation, not since the last update. |
I think all is reasonable then. I just had a case where we wanted to prolong TTL anytime the record was inc/dec. We can't do that atomically right now as we have to fetch/store. I agree that its more like nice to have feature than bug, but it could be easily doable by another boolean param after the TTL as TTL would be required in such case, basically reseting TTL on each inc/dec, not only on recreation. In other point of view seeing apc_inc('key', 1, $success, 30) where 30 is TTL and not setting new TTL on inc/dec but only on recreation would also be confusing. |
// Case where key wasnt initialized - probably some default TTL is used
var_dump(apcu_fetch('foo')); // false
var_dump(apcu_inc('foo', 1, $success), $success); // 1, true
var_dump(apcu_fetch('foo')); // 1
// Case where key was initialized with 3 seconds TTL
var_dump(apcu_store('bar', 0, 3)); // true
var_dump(apcu_inc('bar', 1, $success), $success); // 1, true
var_dump(apcu_fetch('bar')); // 1
Do another request after 3+ seconds on a web page with this code
// This increments and fetches correctly
var_dump(apcu_fetch('foo')); // 1
var_dump(apcu_inc('foo', 1, $success), $success); // 2, true
// This increments but is expired already
var_dump(apcu_fetch('bar')); // false
var_dump(apcu_inc('bar', 1, $success), $success); // 2, true
There are couple of things to keep in mind.
APCu 5.1.4 / PHP 7.0.6
The text was updated successfully, but these errors were encountered: