From 3d758d74956fb52cff50479e3f0f6545850df49d Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 28 Aug 2018 22:14:25 +0100 Subject: [PATCH] jobqueue: Use explicit retry when refreshLinks can't get a lock While RefreshLinksJob is de-duplicated by page-id, it is possible for two jobs to run for the same page ID if the second one was queued after the first one started running. In that case they the newer one must not be skipped or ignored because it will have newer information to record to the database, but it also has no way to stop the old one, and we can't run them concurrently. Instead of letting the lock exception mark the job as error, making it implicitly retry, do this more explicitly, which avoids logspam. Bug: T170596 Co-Authored-By: Aaron Schulz Change-Id: Id2852d73d00daf83f72cf5ff778c638083f5fc73 --- includes/deferred/LinksDeletionUpdate.php | 3 +++ includes/deferred/LinksUpdate.php | 14 +++++++++++--- includes/jobqueue/jobs/DeleteLinksJob.php | 4 ++++ includes/jobqueue/jobs/RefreshLinksJob.php | 5 +++++ 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/includes/deferred/LinksDeletionUpdate.php b/includes/deferred/LinksDeletionUpdate.php index f370e43ffd..5ab83c62c6 100644 --- a/includes/deferred/LinksDeletionUpdate.php +++ b/includes/deferred/LinksDeletionUpdate.php @@ -71,6 +71,9 @@ class LinksDeletionUpdate extends DataUpdate implements EnqueueableDataUpdate { // Make sure all links update threads see the changes of each other. // This handles the case when updates have to batched into several COMMITs. $scopedLock = LinksUpdate::acquirePageLock( $this->getDB(), $id ); + if ( !$scopedLock ) { + throw new RuntimeException( "Could not acquire lock for page ID '{$id}'." ); + } } $title = $this->page->getTitle(); diff --git a/includes/deferred/LinksUpdate.php b/includes/deferred/LinksUpdate.php index 5b1be6d2a5..dbe387be73 100644 --- a/includes/deferred/LinksUpdate.php +++ b/includes/deferred/LinksUpdate.php @@ -21,6 +21,7 @@ */ use Wikimedia\Rdbms\IDatabase; +use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; use Wikimedia\ScopedCallback; @@ -163,6 +164,9 @@ class LinksUpdate extends DataUpdate implements EnqueueableDataUpdate { // Make sure all links update threads see the changes of each other. // This handles the case when updates have to batched into several COMMITs. $scopedLock = self::acquirePageLock( $this->getDB(), $this->mId ); + if ( !$scopedLock ) { + throw new RuntimeException( "Could not acquire lock for page ID '{$this->mId}'." ); + } } // Avoid PHP 7.1 warning from passing $this by reference @@ -190,15 +194,19 @@ class LinksUpdate extends DataUpdate implements EnqueueableDataUpdate { * @param IDatabase $dbw * @param int $pageId * @param string $why One of (job, atomicity) - * @return ScopedCallback - * @throws RuntimeException + * @return ScopedCallback|null * @since 1.27 */ 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 '$key'." ); + $logger = LoggerFactory::getInstance( 'SecondaryDataUpdate' ); + $logger->info( "Could not acquire lock '{key}' for page ID '{page_id}'.", [ + 'key' => $key, + 'page_id' => $pageId, + ] ); + return null; } return $scopedLock; diff --git a/includes/jobqueue/jobs/DeleteLinksJob.php b/includes/jobqueue/jobs/DeleteLinksJob.php index 0a1419212b..8328abfb6b 100644 --- a/includes/jobqueue/jobs/DeleteLinksJob.php +++ b/includes/jobqueue/jobs/DeleteLinksJob.php @@ -47,6 +47,10 @@ class DeleteLinksJob extends Job { // Serialize links updates by page ID so they see each others' changes $scopedLock = LinksUpdate::acquirePageLock( wfGetDB( DB_MASTER ), $pageId, 'job' ); + if ( $scopedLock === null ) { + $this->setLastError( 'LinksUpdate already running for this page, try again later.' ); + return false; + } if ( WikiPage::newFromID( $pageId, WikiPage::READ_LATEST ) ) { // The page was restored somehow or something went wrong diff --git a/includes/jobqueue/jobs/RefreshLinksJob.php b/includes/jobqueue/jobs/RefreshLinksJob.php index 8854c6560f..aa8e121d87 100644 --- a/includes/jobqueue/jobs/RefreshLinksJob.php +++ b/includes/jobqueue/jobs/RefreshLinksJob.php @@ -146,6 +146,11 @@ class RefreshLinksJob extends Job { $dbw = $lbFactory->getMainLB()->getConnection( DB_MASTER ); /** @noinspection PhpUnusedLocalVariableInspection */ $scopedLock = LinksUpdate::acquirePageLock( $dbw, $page->getId(), 'job' ); + if ( $scopedLock === null ) { + // Another job is already updating the page, likely for an older revision (T170596). + $this->setLastError( 'LinksUpdate already running for this page, try again later.' ); + return false; + } // 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. -- 2.20.1