Doctrine 1
  1. Doctrine 1
  2. DC-460

Doctrine_Cache_Apc/Core changes in 1.2 cause instability and crashes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.2.0, 1.2.1
    • Fix Version/s: 1.2.2
    • Component/s: Caching
    • Labels:
      None

      Description

      Since upgrading to Doctrine 1.2 my application has begun to get increasingly slower as hours elapse. I've pinpointed the cause to the new doctrine_cache_keys feature.

      I've found at least three issues with this new method.

      First

      The key cache does not track enforce unique entries, thus multiple entries can be in the cache. See:

          protected function _saveKey($key)
          {
              $keys = $this->fetch($this->_cacheKeyIndexKey);
              $keys[] = $key;
      
              return $this->save($this->_cacheKeyIndexKey, $keys, null, false);
          }
      
          public function _deleteKey($key)
          {
              $keys = $this->fetch($this->_cacheKeyIndexKey);
              $key = array_search($key, $keys);
              if ($key !== false) {
                  unset($keys[$key]);
      
                  return $this->save($this->_cacheKeyIndexKey, $keys, null, false);
              }
      
              return false;
          }
      

      Second

      The doctrine_cache_key is being updated on every cache save or delete. With any caching strategy (APC, Memcached, Xcache) cache writes for the same key are (naturally) serialized. This leads to the "timebomb" situation described here: http://t3.dotgnu.info/blog/php/user-cache-timebomb.html

      The problem is exarcerbated by the infinitely increasing size of the doctrine_cache_key array noted above. Depending on the caching engine, the lock time increases as well (APC & XCache free the previous entry inside the lock). This causes the application to spiral out of control, even with relatively trivial loads.

      Personally, I don't think the "benefit" of having prefix/regex deletes is worth having this. Jon, you suggested having it disabled by default, but even with it enabled, this problem is reintroduced when someone decides to use it.

      Third

      For most (maybe all?) cache engines the doctrine_cache_key seems entirely unnecessary.

      1. APC
        • apc_cache_info('user') returns the list of cache entries
      2. Memcached
        • $memcache->getExtendedStats('cachedump', $slabId) can return the list of cache keys
      3. XCache
        • According to the API docs xcache_list() behaves similar to apc_cache_info()
      4. DB
        • a simple query can get this
      5. Array
        • array_keys() ?

      Summary

      As this is currently broken for production use I intend to immediately fix both the first and second items above by preventing duplicate entries and disabling the doctrine_cache_keys behavior by default.

      With more input/discussion I think the long term solution is to use the tools provided by the engines to get a list of keys, rather than maintain a list. I'd be willing to develop this as long as it is approved.

        Issue Links

          Activity

          Hide
          David Abdemoulaie added a comment - - edited

          I fixed the first issue in the following revision:

          http://trac.doctrine-project.org/changeset/7070

          cache_keys entries are now stored as the key of an array. This allows for simple uniqueness enforcement, as well as being significantly faster for deletion: array_key_exists is O(log n), array_search is O( n ).

          Show
          David Abdemoulaie added a comment - - edited I fixed the first issue in the following revision: http://trac.doctrine-project.org/changeset/7070 cache_keys entries are now stored as the key of an array. This allows for simple uniqueness enforcement, as well as being significantly faster for deletion: array_key_exists is O(log n), array_search is O( n ).
          Hide
          Roman S. Borschel added a comment -

          I have to agree with this feature turning out to be extremely problematic. See DC-390 for a related problem (I linked it to this one). It should really be disabled by default, at least. Of course it can not be stripped out of 1.2.

          We will strongly reconsider it in 2.0.

          Show
          Roman S. Borschel added a comment - I have to agree with this feature turning out to be extremely problematic. See DC-390 for a related problem (I linked it to this one). It should really be disabled by default, at least. Of course it can not be stripped out of 1.2. We will strongly reconsider it in 2.0.
          Hide
          David Abdemoulaie added a comment -

          Jon,

          I was about to backport the 2.0 code to allow enabling/disabling the use of doctrine_cache_keys, but I noticed that it is already in 1.2 at the last second:

              public function save($id, $data, $lifeTime = false, $saveKey = true)
          

          I think the 2.0 method is the better way to go about doing this, because afaik users generally don't call save() directly, the Doctrine code does. Thus there's really no way to disable this class-wide.

          I don't know if such an API change is acceptable to you in 1.2 though. I think it should be done due to the basic unusability of 1.2 caching in production.

          If not, can we just change the $saveKey param default to false?

          Show
          David Abdemoulaie added a comment - Jon, I was about to backport the 2.0 code to allow enabling/disabling the use of doctrine_cache_keys, but I noticed that it is already in 1.2 at the last second: public function save($id, $data, $lifeTime = false, $saveKey = true) I think the 2.0 method is the better way to go about doing this, because afaik users generally don't call save() directly, the Doctrine code does. Thus there's really no way to disable this class-wide. I don't know if such an API change is acceptable to you in 1.2 though. I think it should be done due to the basic unusability of 1.2 caching in production. If not, can we just change the $saveKey param default to false?
          Hide
          David Abdemoulaie added a comment -

          Per discussion on IRC I have completely removed the doctrine_cache_keys functionality from Doctrine_Cache.

          The deleteByPrefix(), deleteBySuffix(), and deleteByRegex() methods are still present, but they use driver-specific methods to retrieve a list of cache keys. Due to this the following methods have been deprecated and removed:

          • Doctrine_Cache_Driver::count() — Counting is no longer possible or relevant because the items cached by Doctrine are indistinguishable from items cached from other parts of your application(s)
          • Doctrine_Cache_Driver::deleteAll() — Same as above.

          The only other API change made is the signature of the Doctrine_Cache_Interface::save() methods:

              public function save($id, $data, $lifeTime = false, $saveKey = true)
          

          is now

              public function save($id, $data, $lifeTime = false)
          

          See r7074 and r7075

          Show
          David Abdemoulaie added a comment - Per discussion on IRC I have completely removed the doctrine_cache_keys functionality from Doctrine_Cache. The deleteByPrefix() , deleteBySuffix() , and deleteByRegex() methods are still present, but they use driver-specific methods to retrieve a list of cache keys. Due to this the following methods have been deprecated and removed: Doctrine_Cache_Driver::count() — Counting is no longer possible or relevant because the items cached by Doctrine are indistinguishable from items cached from other parts of your application(s) Doctrine_Cache_Driver::deleteAll() — Same as above. The only other API change made is the signature of the Doctrine_Cache_Interface::save() methods: public function save($id, $data, $lifeTime = false, $saveKey = true) is now public function save($id, $data, $lifeTime = false) See r7074 and r7075
          Hide
          David Abdemoulaie added a comment -

          Ported changes to 2.0 trunk in r7076

          Show
          David Abdemoulaie added a comment - Ported changes to 2.0 trunk in r7076

            People

            • Assignee:
              Jonathan H. Wage
              Reporter:
              David Abdemoulaie
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: