From: Aaron Schulz Date: Fri, 15 Jul 2016 20:35:03 +0000 (-0700) Subject: Improvements to RefreshLinksJob/DeleteLinksJob locking X-Git-Tag: 1.31.0-rc.0~6338^2 X-Git-Url: http://git.cyclocoop.org/%24href?a=commitdiff_plain;h=63a3911a67507731695bad3188f486219a563b7d;p=lhc%2Fweb%2Fwiklou.git 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 d4a61faf86..22944eb7a6 100644 --- 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 ca5d534f40..f39f8fde05 100644 --- 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 c76ea4f9ab..8fba72894b 100644 --- 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 );