Improvements to RefreshLinksJob/DeleteLinksJob locking

* Removed the lockAndGetLatest() call which caused contention problems.
  Previously, job #2 could block on job #1 in that method, then job #1
  yields the row lock to job #2 in LinksUpdate::acquirePageLock() by
  committing, then job #1 blocks on job #2 in updateLinksTimestamp().
  This caused timeout errors. It also is not fully safe ever since
  batching and acquirePageLock() was added.
* Add an outer getScopedLockAndFlush() call to runForTitle() which
  avoids this contention (as well as contention with page edits)
  but still prevents older jobs from clobbering newer jobs. Edits
  can happen concurrently, since they will enqueue a job post-commit
  that will block on the lock.
* Use the same lock in DeleteLinksJob to edit/deletion races.

Change-Id: I9e2d1eefd7cbb3d2f333c595361d070527d6f0c5

diff --git a/includes/deferred/LinksUpdate.php b/includes/deferred/LinksUpdate.php
index d4a61fa..22944eb 100644 (file)
--- a/includes/deferred/LinksUpdate.php
+++ b/includes/deferred/LinksUpdate.php
@@ -168,18 +168,16 @@ class LinksUpdate extends SqlDataUpdate implements EnqueueableDataUpdate {
         *
         * @param IDatabase $dbw
         * @param integer $pageId
-        * @return ScopedCallback|null Returns null on failure
+        * @param string $why One of (job, atomicity)
+        * @return ScopedCallback
         * @throws RuntimeException
         * @since 1.27
         */
-       public static function acquirePageLock( IDatabase $dbw, $pageId ) {
-               $scopedLock = $dbw->getScopedLockAndFlush(
-                       "LinksUpdate:pageid:$pageId",
-                       __METHOD__,
-                       15
-               );
+       public static function acquirePageLock( IDatabase $dbw, $pageId, $why = 'atomicity' ) {
+               $key = "LinksUpdate:$why:pageid:$pageId";
+               $scopedLock = $dbw->getScopedLockAndFlush( $key, __METHOD__, 15 );
                if ( !$scopedLock ) {
-                       throw new RuntimeException( "Could not acquire lock on page #$pageId." );
+                       throw new RuntimeException( "Could not acquire lock '$key'." );
                }
 
                return $scopedLock;

diff --git a/includes/jobqueue/jobs/DeleteLinksJob.php b/includes/jobqueue/jobs/DeleteLinksJob.php
index ca5d534..f39f8fd 100644 (file)
--- a/includes/jobqueue/jobs/DeleteLinksJob.php
+++ b/includes/jobqueue/jobs/DeleteLinksJob.php
@@ -42,6 +42,10 @@ class DeleteLinksJob extends Job {
                }
 
                $pageId = $this->params['pageId'];
+
+               // Serialize links updates by page ID so they see each others' changes
+               $scopedLock = LinksUpdate::acquirePageLock( wfGetDB( DB_MASTER ), $pageId, 'job' );
+
                if ( WikiPage::newFromID( $pageId, WikiPage::READ_LATEST ) ) {
                        // The page was restored somehow or something went wrong
                        $this->setLastError( "deleteLinks: Page #$pageId exists" );

diff --git a/includes/jobqueue/jobs/RefreshLinksJob.php b/includes/jobqueue/jobs/RefreshLinksJob.php
index c76ea4f..8fba728 100644 (file)
--- a/includes/jobqueue/jobs/RefreshLinksJob.php
+++ b/includes/jobqueue/jobs/RefreshLinksJob.php
@@ -128,8 +128,18 @@ class RefreshLinksJob extends Job {
         * @return bool
         */
        protected function runForTitle( Title $title ) {
+               $stats = MediaWikiServices::getInstance()->getStatsdDataFactory();
+
                $page = WikiPage::factory( $title );
                $page->loadPageData( WikiPage::READ_LATEST );
+
+               // Serialize links updates by page ID so they see each others' changes
+               $scopedLock = LinksUpdate::acquirePageLock( wfGetDB( DB_MASTER ), $page->getId(), 'job' );
+               // Get the latest ID *after* acquirePageLock() flushed the transaction.
+               // This is used to detect edits/moves after loadPageData() but before the scope lock.
+               // The works around the chicken/egg problem of determining the scope lock key.
+               $latest = $title->getLatestRevID( Title::GAID_FOR_UPDATE );
+
                if ( !empty( $this->params['triggeringRevisionId'] ) ) {
                        // Fetch the specified revision; lockAndGetLatest() below detects if the page
                        // was edited since and aborts in order to avoid corrupting the link tables
@@ -142,15 +152,15 @@ class RefreshLinksJob extends Job {
                        $revision = Revision::newFromTitle( $title, false, Revision::READ_LATEST );
                }
 
-               $stats = MediaWikiServices::getInstance()->getStatsdDataFactory();
-
                if ( !$revision ) {
                        $stats->increment( 'refreshlinks.rev_not_found' );
                        $this->setLastError( "Revision not found for {$title->getPrefixedDBkey()}" );
                        return false; // just deleted?
-               } elseif ( !$revision->isCurrent() || $revision->getPage() != $page->getId() ) {
-                       // If the revision isn't current, there's no point in doing a bunch
-                       // of work just to fail at the lockAndGetLatest() check later.
+               } elseif ( $revision->getId() != $latest || $revision->getPage() !== $page->getId() ) {
+                       // Do not clobber over newer updates with older ones. If all jobs where FIFO and
+                       // serialized, it would be OK to update links based on older revisions since it
+                       // would eventually get to the latest. Since that is not the case (by design),
+                       // only update the link tables to a state matching the current revision's output.
                        $stats->increment( 'refreshlinks.rev_not_current' );
                        $this->setLastError( "Revision {$revision->getId()} is not current" );
                        return false;
@@ -250,17 +260,6 @@ class RefreshLinksJob extends Job {
                        }
                }
 
-               $latestNow = $page->lockAndGetLatest();
-               if ( !$latestNow || $revision->getId() != $latestNow ) {
-                       // Do not clobber over newer updates with older ones. If all jobs where FIFO and
-                       // serialized, it would be OK to update links based on older revisions since it
-                       // would eventually get to the latest. Since that is not the case (by design),
-                       // only update the link tables to a state matching the current revision's output.
-                       $stats->increment( 'refreshlinks.rev_cas_failure' );
-                       $this->setLastError( "page_latest changed from {$revision->getId()} to $latestNow" );
-                       return false;
-               }
-
                DataUpdate::runUpdates( $updates );
 
                InfoAction::invalidateCache( $title );