Avoid "Unable to set value to APCBagOStuff" exceptions

* This can happen due to incr/add races. Use incrWithInit()
  instead to handle such cases.
* Also made BagOStuff:incrWithInit() return the new value like incr().

Change-Id: I0e3b02a4cff7c20544a9db2eaabd3f61e5a470b1

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index 703c195..b9be43d 100644 (file)
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -507,18 +507,27 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
        /**
         * Increase stored value of $key by $value while preserving its TTL
         *
-        * This will create the key with value $init and TTL $ttl if not present
+        * This will create the key with value $init and TTL $ttl instead if not present
         *
         * @param string $key
         * @param int $ttl
         * @param int $value
         * @param int $init
-        * @return bool
+        * @return int|bool New value or false on failure
         * @since 1.24
         */
        public function incrWithInit( $key, $ttl, $value = 1, $init = 1 ) {
-               return $this->incr( $key, $value ) ||
-                       $this->add( $key, (int)$init, $ttl ) || $this->incr( $key, $value );
+               $newValue = $this->incr( $key, $value );
+               if ( $newValue === false ) {
+                       // No key set; initialize
+                       $newValue = $this->add( $key, (int)$init, $ttl ) ? $init : false;
+               }
+               if ( $newValue === false ) {
+                       // Raced out initializing; increment
+                       $newValue = $this->incr( $key, $value );
+               }
+
+               return $newValue;
        }
 
        /**

diff --git a/includes/utils/UIDGenerator.php b/includes/utils/UIDGenerator.php
index e2de900..ed7ddb8 100644 (file)
--- a/includes/utils/UIDGenerator.php
+++ b/includes/utils/UIDGenerator.php
@@ -373,12 +373,9 @@ class UIDGenerator {
                        $cache = ObjectCache::getLocalServerInstance();
                }
                if ( $cache ) {
-                       $counter = $cache->incr( $bucket, $count );
+                       $counter = $cache->incrWithInit( $bucket, $cache::TTL_INDEFINITE, $count, $count );
                        if ( $counter === false ) {
-                               if ( !$cache->add( $bucket, (int)$count ) ) {
-                                       throw new RuntimeException( 'Unable to set value to ' . get_class( $cache ) );
-                               }
-                               $counter = $count;
+                               throw new RuntimeException( 'Unable to set value to ' . get_class( $cache ) );
                        }
                }
 

diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
index 94b74cb..b9fd6ab 100644 (file)
--- a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
@@ -183,6 +183,18 @@ class BagOStuffTest extends MediaWikiTestCase {
                $this->assertEquals( $expectedValue, $actualValue, 'Value should be 1 after incrementing' );
        }
 
+       /**
+        * @covers BagOStuff::incrWithInit
+        */
+       public function testIncrWithInit() {
+               $key = wfMemcKey( 'test' );
+               $val = $this->cache->incrWithInit( $key, 0, 1, 3 );
+               $this->assertEquals( 3, $val, "Correct init value" );
+
+               $val = $this->cache->incrWithInit( $key, 0, 1, 3 );
+               $this->assertEquals( 4, $val, "Correct init value" );
+       }
+
        /**
         * @covers BagOStuff::getMulti
         */