Merge "Added CAS-style logic to User::saveSettings as a final sanity check"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Tue, 7 Apr 2015 15:43:43 +0000 (15:43 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Tue, 7 Apr 2015 15:43:43 +0000 (15:43 +0000)
1  2 
includes/User.php

diff --combined includes/User.php
@@@ -452,7 -452,7 +452,7 @@@ class User implements IDBAccessObject 
                // The cache needs good consistency due to its high TTL, so the user
                // should have been loaded from the master to avoid lag amplification.
                if ( !( $this->queryFlagsUsed & self::READ_LATEST ) ) {
-                       wfWarn( "Cannot save slave-loaded User object data to cache." );
+                       wfWarn( "Cannot cache slave-loaded User object with ID '{$this->mId}'." );
                        return;
                }
  
                global $wgAuth;
  
                if ( wfReadOnly() ) {
 -                      return; // @TODO: caller should deal with this instead!
 +                      // @TODO: caller should deal with this instead!
 +                      // This should really just be an exception.
 +                      MWExceptionHandler::logException( new DBExpectedError(
 +                              null,
 +                              "Could not update user with ID '{$this->mId}'; DB is read-only."
 +                      ) );
 +                      return;
                }
  
                $this->load();
                $this->loadPasswords();
                if ( 0 == $this->mId ) {
-                       return;
+                       return; // anon
                }
  
                // This method is for updating existing users, so the user should
                // have been loaded from the master to begin with to avoid problems.
                if ( !( $this->queryFlagsUsed & self::READ_LATEST ) ) {
-                       wfWarn( "Attempting to save slave-loaded User object data." );
+                       wfWarn( "Attempting to save slave-loaded User object with ID '{$this->mId}'." );
                }
  
+               // Get a new user_touched that is higher than the old one.
+               // This will be used for a CAS check as a last-resort safety
+               // check against race conditions and slave lag.
+               $oldTouched = $this->mTouched;
                $this->mTouched = $this->newTouchedTimestamp();
                if ( !$wgAuth->allowSetLocalPassword() ) {
                        $this->mPassword = self::getPasswordFactory()->newFromCiphertext( null );
                }
                                'user_email_token_expires' => $dbw->timestampOrNull( $this->mEmailTokenExpires ),
                                'user_password_expires' => $dbw->timestampOrNull( $this->mPasswordExpires ),
                        ), array( /* WHERE */
-                               'user_id' => $this->mId
+                               'user_id' => $this->mId,
+                               'user_touched' => $dbw->timestamp( $oldTouched ) // CAS check
                        ), __METHOD__
                );
  
+               if ( !$dbw->affectedRows() ) {
+                       // User was changed in the meantime or loaded with stale data
+                       MWExceptionHandler::logException( new MWException(
+                               "CAS update failed on user_touched for user ID '{$this->mId}'."
+                       ) );
+                       // Maybe the problem was a missed cache update; clear it to be safe
+                       $this->clearSharedCache();
+                       return;
+               }
                $this->saveOptions();
  
                Hooks::run( 'UserSaveSettings', array( $this ) );