Bug in CMemCache.php #2164

Closed
ardicoetzee opened this Issue Mar 4, 2013 · 14 comments

Projects

None yet

8 participants

@ardicoetzee

Little more details can be found here: http://stackoverflow.com/questions/14954778/yii-time-caching-not-working/14956296

But in a nutshell... this:

protected function setValue($key,$value,$expire)
{
  if($expire>0)
    $expire+=time();
  else
    $expire=0;

  return $this->useMemcached ? $this->_cache->set($key,$value,$expire) : $this->_cache->set($key,$value,0,$expire);
}

Must be changed to:

protected function setValue($key,$value,$expire)
{
  return $this->useMemcached ? $this->_cache->set($key,$value,$expire) : $this->_cache->set($key,$value,0,$expire);
}

Because Memcache does not want the time to be added to expire, but rather just the seconds from now that it must expire.

@samdark
Member
samdark commented Mar 4, 2013

That's not entirely true http://php.net/manual/en/memcache.set.php

Expiration time of the item. If it's equal to zero, the item will never expire. You can also use Unix timestamp or a number of seconds starting from current time, but in the latter case the number of seconds may not exceed 2592000 (30 days).

@ardicoetzee

It is entirely true. If you use the function as it is now. by saying:

setValue('bla', 'bla' , 60);

... it will not expire in 60 seconds, but in 60 seconds plus 1362389997 seconds. Which is more than the max. In other words, the function is wrong. It must not add time() to it, or it will not be an acceptable value. It must take only the value 60 and pass that to the memcache function.

@ardicoetzee

I do agree that provision must be made for if the user enters a value smaller than 0, or a non-numerical value, if you want to get technical. But what I'm trying to point out is the bug with the +time() part.

@samdark samdark was assigned Mar 4, 2013
@samdark
Member
samdark commented Mar 4, 2013

And in case of memcached: http://www.php.net/manual/en/memcached.expiration.php

may either be Unix time (number of seconds since January 1, 1970, as an integer), or a number of seconds starting from current time. In the latter case, this number of seconds may not exceed 60_60_24*30 (number of seconds in 30 days); if the expiration value is larger than that, the server will consider it to be real Unix time value rather than an offset from current time.

Note that if you pass the expiration time as an offset of seconds then the cache item will expire in current-second + offset, not in now + offset.

@samdark
Member
samdark commented Mar 4, 2013

@yodacoda85 documentation for both memcache and memcached says You can also use Unix timestamp

@ardicoetzee

Then there's a bug in memcache, because it doesn't work like that. I'll check what versions I'm using and try to get to the bottom of it.

@youmee
youmee commented May 30, 2013

I have the same issue. It seems like it is a bug in Windows binary version. I'm using memcached on windows and have the same bug. But the same code works well on linux machine.

@samdark
Member
samdark commented Jun 1, 2013

I have Windows build w/o the bug. Seems to be memcached issue specific for some very uncommon builds. I don't think we should change anything.

@samdark samdark closed this Jun 1, 2013
@jandetlefsen

We encountered the same issue on multiple Debian Linux machines, it doesn't work when time() gets added to the interval. This is a very common setup.

@kievechua

I also confirm on our side having the same issue for memcache.

@rtvitalmaksim

I've faced the same issue. It would be good if you add another option useExpireTimestamp to let us change the behavior of setValue method.

Memcache version - STAT version 1.4.13

The following code

<?php
$memcache_obj = new Memcache;
$memcache_obj->connect('127.0.0.1', 11211);
$memcache_obj->set('var_key', 'some really big variable', 0, time() + 1);
var_dump($memcache_obj->get('var_key'));
sleep(2);
var_dump($memcache_obj->get('var_key'));
?>

outputs:

bool(false)
bool(false)

and the code

<?php
$memcache_obj = new Memcache;
$memcache_obj->connect('127.0.0.1', 11211);
$memcache_obj->set('var_key', 'some really big variable', 0, 1);
var_dump($memcache_obj->get('var_key'));
sleep(2);
var_dump($memcache_obj->get('var_key'));
?>

outputs:

string(24) "some really big variable"
bool(false)
@dizeee
dizeee commented Sep 16, 2014

Sasha, as you fairly noticed, the documentation provides two different ways to set expiration time. I understand that you don't want to brake the CCache interface for the sake of just one implementation. But:

  1. This is a bug. Nobody cares whether it is in the documentation or the memcached or the framework.
  2. It reproduces on a variety of debian-based distributions. No offense, but I really don't think that most of Yii users write their apps for Windows.
  3. Memcache for 30+ days? Really!?

My point is, that for now those who know about this bug are forced to make their own implementations, and those who don't - just produce buggy apps.

@dizeee
dizeee commented Sep 16, 2014

The most voted comment for now on http://php.net/manual/en/memcache.set.php:

The max time for expiration (without having to worry about deletions when necessary as with 0 seconds) is 2,592,000 seconds (30 days).

Specifying an expiration value above that will return false, but will NOT throw in error so it is easy to miss.

@boazrymland

Just hit the same issue. I use memcached on Linux (stock Ubuntu 14.04-2 packages), Yii v1.1.16.

I needed to use a timestamp of 'this midnight' as the expiration date for the cache record. Output of the convenient command

strtotime('tomorrow midnight');

When I used it, the cache record didn't stay in the cache at all. Short dive into CMemCache.setValue() discovered what's described above. Actually, I have my own child class that extends CMemCache and supports memcached only (and adds support for 'cas()' method of memcached, which was the reasoning for this extension). So I changed the setValue() code to simply:

protected function setValue($key, $value, $expire = 0)
    {
        return $this->_cache->set($key, $value, $expire);
    }

And now it functions as expected.

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