From 140da10741c9850fa58c06f28003c3216b6f27a5 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Tue, 19 Jul 2016 12:51:57 -0700
Subject: [PATCH] Move updateNotificationTimestamp callback to DeferredUpdates

* This puts the complex logic here after the commit step for
  all DBs, making the main multi-DB transaction more likely
  to be atomic.
* Also fixed the reuseConnection() call by getting a new
  handle in the callback.

Change-Id: I449a521423ff13bfbf49bdaa6e7e6df2145c8751
---
 includes/WatchedItemStore.php                 | 25 ++++++------
 .../includes/WatchedItemStoreUnitTest.php     | 40 +++++--------------
 2 files changed, 22 insertions(+), 43 deletions(-)

diff --git a/includes/WatchedItemStore.php b/includes/WatchedItemStore.php
index 515fbfce86..89ca50c00b 100644
--- a/includes/WatchedItemStore.php
+++ b/includes/WatchedItemStore.php
@@ -719,28 +719,29 @@ class WatchedItemStore implements StatsdAwareInterface {
 	 */
 	public function updateNotificationTimestamp( User $editor, LinkTarget $target, $timestamp ) {
 		$dbw = $this->getConnection( DB_MASTER );
-		$res = $dbw->select( [ 'watchlist' ],
-			[ 'wl_user' ],
+		$uids = $dbw->selectFieldValues(
+			'watchlist',
+			'wl_user',
 			[
 				'wl_user != ' . intval( $editor->getId() ),
 				'wl_namespace' => $target->getNamespace(),
 				'wl_title' => $target->getDBkey(),
 				'wl_notificationtimestamp IS NULL',
-			], __METHOD__
+			],
+			__METHOD__
 		);
+		$this->reuseConnection( $dbw );
 
-		$watchers = [];
-		foreach ( $res as $row ) {
-			$watchers[] = intval( $row->wl_user );
-		}
-
+		$watchers = array_map( 'intval', $uids );
 		if ( $watchers ) {
 			// Update wl_notificationtimestamp for all watching users except the editor
 			$fname = __METHOD__;
-			$dbw->onTransactionIdle(
-				function () use ( $dbw, $timestamp, $watchers, $target, $fname ) {
+			DeferredUpdates::addCallableUpdate(
+				function () use ( $timestamp, $watchers, $target, $fname ) {
 					global $wgUpdateRowsPerQuery;
 
+					$dbw = $this->getConnection( DB_MASTER );
+
 					$watchersChunks = array_chunk( $watchers, $wgUpdateRowsPerQuery );
 					foreach ( $watchersChunks as $watchersChunk ) {
 						$dbw->update( 'watchlist',
@@ -758,12 +759,12 @@ class WatchedItemStore implements StatsdAwareInterface {
 						}
 					}
 					$this->uncacheLinkTarget( $target );
+
+					$this->reuseConnection( $dbw );
 				}
 			);
 		}
 
-		$this->reuseConnection( $dbw );
-
 		return $watchers;
 	}
 
diff --git a/tests/phpunit/includes/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/WatchedItemStoreUnitTest.php
index 2d2e726c83..030d9d59db 100644
--- a/tests/phpunit/includes/WatchedItemStoreUnitTest.php
+++ b/tests/phpunit/includes/WatchedItemStoreUnitTest.php
@@ -2444,10 +2444,10 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
 	public function testUpdateNotificationTimestamp_watchersExist() {
 		$mockDb = $this->getMockDb();
 		$mockDb->expects( $this->once() )
-			->method( 'select' )
+			->method( 'selectFieldValues' )
 			->with(
-				[ 'watchlist' ],
-				[ 'wl_user' ],
+				'watchlist',
+				'wl_user',
 				[
 					'wl_user != 1',
 					'wl_namespace' => 0,
@@ -2455,18 +2455,7 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
 					'wl_notificationtimestamp IS NULL'
 				]
 			)
-			->will(
-				$this->returnValue( [
-					$this->getFakeRow( [ 'wl_user' => '2' ] ),
-					$this->getFakeRow( [ 'wl_user' => '3' ] )
-				] )
-			);
-		$mockDb->expects( $this->once() )
-			->method( 'onTransactionIdle' )
-			->with( $this->isType( 'callable' ) )
-			->will( $this->returnCallback( function( $callable ) {
-				$callable();
-			} ) );
+			->will( $this->returnValue( [ '2', '3' ] ) );
 		$mockDb->expects( $this->once() )
 			->method( 'update' )
 			->with(
@@ -2502,10 +2491,10 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
 	public function testUpdateNotificationTimestamp_noWatchers() {
 		$mockDb = $this->getMockDb();
 		$mockDb->expects( $this->once() )
-			->method( 'select' )
+			->method( 'selectFieldValues' )
 			->with(
-				[ 'watchlist' ],
-				[ 'wl_user' ],
+				'watchlist',
+				'wl_user',
 				[
 					'wl_user != 1',
 					'wl_namespace' => 0,
@@ -2516,8 +2505,6 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
 			->will(
 				$this->returnValue( [] )
 			);
-		$mockDb->expects( $this->never() )
-			->method( 'onTransactionIdle' );
 		$mockDb->expects( $this->never() )
 			->method( 'update' );
 
@@ -2551,19 +2538,10 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
 				$this->getFakeRow( [ 'wl_notificationtimestamp' => '20151212010101' ] )
 			) );
 		$mockDb->expects( $this->once() )
-			->method( 'select' )
+			->method( 'selectFieldValues' )
 			->will(
-				$this->returnValue( [
-					$this->getFakeRow( [ 'wl_user' => '2' ] ),
-					$this->getFakeRow( [ 'wl_user' => '3' ] )
-				] )
+				$this->returnValue( [ '2', '3' ] )
 			);
-		$mockDb->expects( $this->once() )
-			->method( 'onTransactionIdle' )
-			->with( $this->isType( 'callable' ) )
-			->will( $this->returnCallback( function( $callable ) {
-				$callable();
-			} ) );
 		$mockDb->expects( $this->once() )
 			->method( 'update' );
 
-- 
2.20.1