Merge "Avoid use of rollback() in WikiPage::doEditContent()"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Tue, 3 Nov 2015 23:36:53 +0000 (23:36 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Tue, 3 Nov 2015 23:36:53 +0000 (23:36 +0000)
1  2 
includes/page/WikiPage.php

@@@ -1242,7 -1242,7 +1242,7 @@@ class WikiPage implements Page, IDBAcce
         *   Giving 0 indicates the new page flag should be set on.
         * @param bool $lastRevIsRedirect If given, will optimize adding and
         *   removing rows in redirect table.
-        * @return bool True on success, false on failure
+        * @return bool Success; false if the page row was missing or page_latest changed
         */
        public function updateRevisionOn( $dbw, $revision, $lastRevision = null,
                $lastRevIsRedirect = null
                        $changed = !$content->equals( $old_content );
  
                        if ( $changed ) {
-                               $dbw->begin( __METHOD__ );
                                $prepStatus = $content->prepareSave( $this, $flags, $oldid, $user );
                                $status->merge( $prepStatus );
  
                                if ( !$status->isOK() ) {
-                                       $dbw->rollback( __METHOD__ );
                                        return $status;
                                }
-                               $revisionId = $revision->insertOn( $dbw );
-                               // Update page.
-                               // We check for conflicts by comparing $oldid with the current latest revision ID.
-                               $ok = $this->updateRevisionOn( $dbw, $revision, $oldid, $oldIsRedirect );
  
-                               if ( !$ok ) {
-                                       // Belated edit conflict! Run away!!
+                               $dbw->begin( __METHOD__ );
+                               // Get the latest page_latest value while locking it.
+                               // Do a CAS style check to see if it's the same as when this method
+                               // started. If it changed then bail out before touching the DB.
+                               $latestNow = $this->lock();
+                               if ( $latestNow != $oldid ) {
+                                       $dbw->commit( __METHOD__ );
+                                       // Page updated or deleted in the mean time
                                        $status->fatal( 'edit-conflict' );
  
-                                       $dbw->rollback( __METHOD__ );
                                        return $status;
                                }
  
+                               // At this point we are now comitted to returning an OK
+                               // status unless some DB query error or other exception comes up.
+                               // This way callers don't have to call rollback() if $status is bad
+                               // unless they actually try to catch exceptions (which is rare).
+                               $revisionId = $revision->insertOn( $dbw );
+                               // Update page_latest and friends to reflect the new revision
+                               $this->updateRevisionOn( $dbw, $revision, null, $oldIsRedirect );
                                Hooks::run( 'NewRevisionFromEditComplete',
                                        array( $this, $revision, $baseRevId, $user ) );
  
                        // Create new article
                        $status->value['new'] = true;
  
-                       $dbw->begin( __METHOD__ );
                        $prepStatus = $content->prepareSave( $this, $flags, $oldid, $user );
                        $status->merge( $prepStatus );
                        if ( !$status->isOK() ) {
-                               $dbw->rollback( __METHOD__ );
                                return $status;
                        }
  
-                       $status->merge( $prepStatus );
+                       $dbw->begin( __METHOD__ );
  
-                       // Add the page record; stake our claim on this title!
-                       // This will return false if the article already exists
+                       // Add the page record unless one already exists for the title
                        $newid = $this->insertOn( $dbw );
                        if ( $newid === false ) {
-                               $dbw->rollback( __METHOD__ );
+                               $dbw->commit( __METHOD__ ); // nothing inserted
                                $status->fatal( 'edit-already-exists' );
  
-                               return $status;
+                               return $status; // nothing done
                        }
  
+                       // At this point we are now comitted to returning an OK
+                       // status unless some DB query error or other exception comes up.
+                       // This way callers don't have to call rollback() if $status is bad
+                       // unless they actually try to catch exceptions (which is rare).
                        // Save the revision text...
                        $revision = new Revision( array(
                                'page'       => $newid,
                        }
  
                        // Update the page record with revision data
-                       $this->updateRevisionOn( $dbw, $revision, 0 );
+                       if ( !$this->updateRevisionOn( $dbw, $revision, 0 ) ) {
+                               $dbw->rollback( __METHOD__ );
+                               throw new MWException( "Failed to update page row to use new revision." );
+                       }
  
                        Hooks::run( 'NewRevisionFromEditComplete', array( $this, $revision, false, $user ) );
  
                        foreach ( $updates as $update ) {
                                if ( $update instanceof LinksUpdate ) {
                                        $update->setRevision( $revision );
 +                                      $update->setTriggeringUser( $user );
                                }
                                DeferredUpdates::addUpdate( $update );
                        }
                $status->value = $logid;
  
                // Show log excerpt on 404 pages rather than just a link
 +              $cache = ObjectCache::getMainStashInstance();
                $key = wfMemcKey( 'page-recent-delete', md5( $logTitle->getPrefixedText() ) );
 -              ObjectCache::getMainStashInstance()->set( $key, 1, 86400 );
 +              $cache->set( $key, 1, $cache::TTL_DAY );
  
                return $status;
        }
                        return;
                }
  
 +              $params = array(
 +                      'isOpportunistic' => true,
 +                      'rootJobTimestamp' => $parserOutput->getCacheTime()
 +              );
 +
                if ( $this->mTitle->areRestrictionsCascading() ) {
                        // If the page is cascade protecting, the links should really be up-to-date
 -                      $params = array( 'prioritize' => true );
 +                      JobQueueGroup::singleton()->lazyPush(
 +                              RefreshLinksJob::newPrioritized( $this->mTitle, $params )
 +                      );
                } elseif ( $parserOutput->hasDynamicContent() ) {
 -                      // Assume the output contains time/random based magic words
 -                      $params = array();
 -              } else {
 -                      // If the inclusions are deterministic, the edit-triggered link jobs are enough
 -                      return;
 -              }
 -
 -              // Check if the last link refresh was before page_touched
 -              if ( $this->getLinksTimestamp() < $this->getTouched() ) {
 -                      $params['isOpportunistic'] = true;
 -                      $params['rootJobTimestamp'] = $parserOutput->getCacheTime();
 -                      JobQueueGroup::singleton()->lazyPush( new RefreshLinksJob( $this->mTitle, $params ) );
 +                      // Assume the output contains "dynamic" time/random based magic words.
 +                      // Only update pages that expired due to dynamic content and NOT due to edits
 +                      // to referenced templates/files. When the cache expires due to dynamic content,
 +                      // page_touched is unchanged. We want to avoid triggering redundant jobs due to
 +                      // views of pages that were just purged via HTMLCacheUpdateJob. In that case, the
 +                      // template/file edit already triggered recursive RefreshLinksJob jobs.
 +                      if ( $this->getLinksTimestamp() > $this->getTouched() ) {
 +                              // If a page is uncacheable, do not keep spamming a job for it.
 +                              // Although it would be de-duplicated, it would still waste I/O.
 +                              $cache = ObjectCache::getLocalClusterInstance();
 +                              $key = $cache->makeKey( 'dynamic-linksupdate', 'last', $this->getId() );
 +                              if ( $cache->add( $key, time(), 60 ) ) {
 +                                      JobQueueGroup::singleton()->lazyPush(
 +                                              RefreshLinksJob::newDynamic( $this->mTitle, $params )
 +                                      );
 +                              }
 +                      }
                }
        }