apcu_inc/apcu_dec increments and returns value even if the key is expired

[fabicz]

// 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.

1. When key is initialized by apcu_inc on non-existent key, then whats the TTL? Could it be possible to have another optional param TTL for inc/dec to define new TTL? If not defined then keep original TTL. It would be handy as touching the key we want to prolong TTL in certain cases.
2. apcu_inc should not increment and return value if TTL of the key expired. If optional TTL in above is provided and key was expired then start over again from 0 (+step) and set TTL to one defined. Otherwise return false.

APCu 5.1.4 / PHP 7.0.6

[rburkat]

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 Definitely not what I was expecting from apc_inc on expired keys.

[PHLAK]

I just ran into this same issue on Ubuntu 16.04.

$ php -a
Interactive mode enabled

php > var_dump(apcu_exists('qwerty'));
bool(false)
php > var_dump(apcu_inc('qwerty', 1));
int(1)
php > var_dump(apcu_exists('qwerty'));
bool(true)

System info:

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04 LTS
Release:    16.04
Codename:   xenial

$ php -v
PHP 7.0.4-7ubuntu2.1 (cli) ( NTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies

$ apt list --installed | grep apcu
php-apcu/xenial,now 5.1.3+4.0.10-1build1 amd64 [installed]

$ pecl list
Installed packages, channel pecl.php.net:
=========================================
Package Version State
apcu    5.1.5   stable
apcu_bc 1.0.3   beta

Edit: I worked around this with the following for now:

return apcu_exists($key) ? apcu_inc($key, $value) : false;

[MorrisJobke]

I ran into the same issue:

PHP 7:

php > var_dump(apcu_exists('abcd'));
php shell code:1:
bool(false)
php > var_dump(apcu_fetch('abcd'));
php shell code:1:
bool(false)
php > var_dump(apcu_dec('abcd'));
php shell code:1:
int(-1)
php > var_dump(apcu_exists('abcd'));
php shell code:1:
bool(true)

$ apt list --installed | grep apcu
php7.0-apcu/jessie,now 5.1.5-1~dotdeb+8.2 amd64 [installed]

In PHP 5.6 this works fine:

php > var_dump(apcu_exists('abcd'));
bool(false)
php > var_dump(apcu_fetch('abcd'));
bool(false)
php > var_dump(apcu_dec('abcd'));
bool(false)
php > var_dump(apcu_exists('abcd'));
bool(false)

To test this we have a PHP 7.0 and a PHP 5.6 docker container with APCu enabled:

$ docker run -ti --rm nextcloudci/php5.6:php5.6-1
$ docker run -ti --rm nextcloudci/php7.0:php7.0-1 

Then run these commands:

php -a
var_dump(apcu_exists('abcd'));
var_dump(apcu_fetch('abcd'));
var_dump(apcu_dec('abcd'));
var_dump(apcu_exists('abcd'));

I now also added this as workaround but having this solved in a consistent way across multiple PHP versions would be nice.

return apcu_exists($key) ? apcu_inc($key, $value) : false;

[krakjoe]

This was a purposeful change that made sense when I reviewed it.

I'll have to think about that ...

[nikic]

I've implemented two changes to address this:

• https://github.com/krakjoe/apcu/commit/ab88120bcd1d6b920b3e79b396ac0224f57a5d6e makes apc_inc/dec respect the per-entry TTL. If the TTL is expired, the entry is treated as if it doesn't exist and a new one will be created.
• https://github.com/krakjoe/apcu/commit/75d5c2831972d35f0492726baf8f3f074ac11b15 adds an additional optional $ttl argument to apc_inc/apc_dec, which will be used when creating the entry if it doesn't exist yet (or is expired). As usual, the TTL defaults to zero.

The original report also suggest to...

• ...inherit the TTL of the expired entry. I chose against this because it will not yield a sensible behavior if the expired entry has been purged in the meantime. This would make the behavior unreliable, as expired entries may be purges as a side-effect of unrelated operations.
• ...not create the entry if TTL is not passed and instead return false. I did not implement this, as it would be inconsistent with the handling of TTL arguments in all other APIs (where lack of TTL implies that the entry lives forever, modulo the global cache TTL). I'm also concerned about changing the (effectively default) behavior here again after it has been this way for so long.

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.

[fabicz]

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?

[nikic]

@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.

[fabicz]

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.