From 5ffaa6ae0dcff70a6ce8a38934c7aa7a2a4e5bff Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 21 Dec 2015 12:12:06 -0800
Subject: [PATCH] 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
---
 includes/libs/objectcache/BagOStuff.php         | 17 +++++++++++++----
 includes/utils/UIDGenerator.php                 |  7 ++-----
 .../includes/libs/objectcache/BagOStuffTest.php | 12 ++++++++++++
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index 703c195a2b..b9be43df8a 100644
--- 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 e2de900a16..ed7ddb8f26 100644
--- 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 94b74cb651..b9fd6ab81f 100644
--- 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
 	 */
-- 
2.20.1