From b8779134ed9a06a587e56f864fbcb803c8bcb374 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 11 Jul 2018 23:54:24 +0100
Subject: [PATCH] objectcache: make BagOStuff::mergeViaLock() timeout more
 sensible

Interpret "1 attempt" as non-blocking and use 3 seconds otherwise
(instead of 6 seconds). This means that the WAN cache merge call
does not block.

Bug: T198239
Change-Id: Ieb194a949f18785c2cc9dd20fdc4d2e3fed51abf
---
 includes/libs/objectcache/BagOStuff.php       |  8 +++++-
 .../libs/objectcache/BagOStuffTest.php        | 28 ++++++++++++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index 0100fb2c41..782f4c618e 100644
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -371,7 +371,13 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
 	 * @return bool Success
 	 */
 	protected function mergeViaLock( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
-		if ( !$this->lock( $key, 6 ) ) {
+		if ( $attempts <= 1 ) {
+			$timeout = 0; // clearly intended to be "non-blocking"
+		} else {
+			$timeout = 3;
+		}
+
+		if ( !$this->lock( $key, $timeout ) ) {
 			return false;
 		}
 
diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
index b6709a0f29..f0f55fb6ac 100644
--- a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
@@ -68,8 +68,11 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers BagOStuff::mergeViaCas
 	 */
 	public function testMerge() {
+		$calls = 0;
 		$key = $this->cache->makeKey( self::TEST_KEY );
-		$callback = function ( BagOStuff $cache, $key, $oldVal ) {
+		$callback = function ( BagOStuff $cache, $key, $oldVal ) use ( &$calls ) {
+			++$calls;
+
 			return ( $oldVal === false ) ? 'merged' : $oldVal . 'merged';
 		};
 
@@ -82,6 +85,12 @@ class BagOStuffTest extends MediaWikiTestCase {
 		$merged = $this->cache->merge( $key, $callback, 5 );
 		$this->assertTrue( $merged );
 		$this->assertEquals( 'mergedmerged', $this->cache->get( $key ) );
+
+		$calls = 0;
+		$this->cache->lock( $key );
+		$this->assertFalse( $this->cache->merge( $key, $callback, 1 ), 'Non-blocking merge' );
+		$this->cache->unlock( $key );
+		$this->assertEquals( 0, $calls );
 	}
 
 	/**
@@ -305,4 +314,21 @@ class BagOStuffTest extends MediaWikiTestCase {
 
 		DeferredUpdates::doUpdates();
 	}
+
+	/**
+	 * @covers BagOStuff::lock()
+	 * @covers BagOStuff::unlock()
+	 */
+	public function testLocking() {
+		$key = 'test';
+		$this->assertTrue( $this->cache->lock( $key ) );
+		$this->assertFalse( $this->cache->lock( $key ) );
+		$this->assertTrue( $this->cache->unlock( $key ) );
+
+		$key2 = 'test2';
+		$this->assertTrue( $this->cache->lock( $key2, 5, 5, 'rclass' ) );
+		$this->assertTrue( $this->cache->lock( $key2, 5, 5, 'rclass' ) );
+		$this->assertTrue( $this->cache->unlock( $key2 ) );
+		$this->assertTrue( $this->cache->unlock( $key2 ) );
+	}
 }
-- 
2.20.1