Added FOR UPDATE mode to Block.php, to fix memcached concurrency problem in BlockCach...

diff --git a/includes/Block.php b/includes/Block.php
index 427fda2..3be78ed 100644 (file)
--- a/includes/Block.php
+++ b/includes/Block.php
@@ -10,10 +10,13 @@
 
 # Globals used: $wgBlockCache, $wgAutoblockExpiry
 
+define ( 'EB_KEEP_EXPIRED', 1 );
+define ( 'EB_FOR_UPDATE', 2 );
+
 class Block
 {
        /* public*/ var $mAddress, $mUser, $mBy, $mReason, $mTimestamp, $mAuto, $mId, $mExpiry;
-       /* private */ var $mNetworkBits, $mIntegerAddr;
+       /* private */ var $mNetworkBits, $mIntegerAddr, $mForUpdate;
        
        function Block( $address = '', $user = '', $by = 0, $reason = '', 
                $timestamp = '' , $auto = 0, $expiry = '' ) 
@@ -25,7 +28,8 @@ class Block
                $this->mTimestamp = $timestamp;
                $this->mAuto = $auto;
                $this->mExpiry = $expiry;
-
+               
+               $this->mForUpdate = false;
                $this->initialiseRange();
        }
        
@@ -49,27 +53,33 @@ class Block
 
                $ret = false;
                $killed = false;
-               $dbr =& wfGetDB( DB_SLAVE );
-               $ipblocks = $dbr->tableName( 'ipblocks' );
+               if ( $this->forUpdate() ) {
+                       $db =& wfGetDB( DB_MASTER );
+                       $options = 'FOR UPDATE';
+               } else {
+                       $db =& wfGetDB( DB_SLAVE );
+                       $options = '';
+               }
+               $ipblocks = $db->tableName( 'ipblocks' );
 
                if ( 0 == $user && $address=="" ) {
-                       $sql = "SELECT * from $ipblocks";
+                       $sql = "SELECT * from $ipblocks $options";
                } elseif ($address=="") {
-                       $sql = "SELECT * FROM $ipblocks WHERE ipb_user={$user}";
+                       $sql = "SELECT * FROM $ipblocks WHERE ipb_user={$user} $options";
                } elseif ($user=="") {
-                       $sql = "SELECT * FROM $ipblocks WHERE ipb_address='" . $dbr->strencode( $address ) . "'";
+                       $sql = "SELECT * FROM $ipblocks WHERE ipb_address='" . $db->strencode( $address ) . "' $options";
                } else {
-                       $sql = "SELECT * FROM $ipblocks WHERE (ipb_address='" . $dbr->strencode( $address ) . 
-                               "' OR ipb_user={$user})";
+                       $sql = "SELECT * FROM $ipblocks WHERE (ipb_address='" . $db->strencode( $address ) . 
+                               "' OR ipb_user={$user}) $options";
                }
 
-               $res = $dbr->query( $sql, $fname );
-               if ( 0 == $dbr->numRows( $res ) ) {
+               $res = $db->query( $sql, $fname );
+               if ( 0 == $db->numRows( $res ) ) {
                        # User is not blocked
                        $this->clear();
                } else {
                        # Get first block
-                       $row = $dbr->fetchObject( $res );
+                       $row = $db->fetchObject( $res );
                        $this->initFromRow( $row );
 
                        if ( $killExpired ) {
@@ -77,7 +87,7 @@ class Block
                                do {
                                        $killed = $this->deleteIfExpired();
                                        if ( $killed ) {
-                                               $row = $dbr->fetchObject( $res );
+                                               $row = $db->fetchObject( $res );
                                                if ( $row ) {
                                                        $this->initFromRow( $row );
                                                }
@@ -95,7 +105,7 @@ class Block
                                $ret = true;
                        }
                }
-               $dbr->freeResult( $res );
+               $db->freeResult( $res );
                return $ret;
        }
        
@@ -130,18 +140,25 @@ class Block
        }
        
        # Callback with a Block object for every block
-       /*static*/ function enumBlocks( $callback, $tag, $killExpired = true ) 
+       /*static*/ function enumBlocks( $callback, $tag, $flags = 0 ) 
        {
-               $dbr =& wfGetDB( DB_SLAVE );
-               $ipblocks = $dbr->tableName( 'ipblocks' );
-               
-               $sql = "SELECT * FROM $ipblocks ORDER BY ipb_timestamp DESC";
-               $res = $dbr->query( $sql, 'Block::enumBans' );
                $block = new Block();
+               if ( $flags & EB_FOR_UPDATE ) {
+                       $db =& wfGetDB( DB_MASTER );
+                       $options = 'FOR UPDATE';
+                       $block->forUpdate( true );
+               } else {
+                       $db =& wfGetDB( DB_SLAVE );
+                       $options = '';
+               }       
+               $ipblocks = $db->tableName( 'ipblocks' );
+               
+               $sql = "SELECT * FROM $ipblocks ORDER BY ipb_timestamp DESC $options";
+               $res = $db->query( $sql, 'Block::enumBans' );
 
-               while ( $row = $dbr->fetchObject( $res ) ) {
+               while ( $row = $db->fetchObject( $res ) ) {
                        $block->initFromRow( $row );
-                       if ( $killExpired ) {
+                       if ( !( $flags & EB_KEEP_EXPIRED ) ) {
                                if ( !$block->deleteIfExpired() ) {
                                        $callback( $block, $tag );
                                }
@@ -232,7 +249,7 @@ class Block
        {
                global $wgBlockCache;
                if ( is_object( $wgBlockCache ) ) {
-                       $wgBlockCache->clear();
+                       $wgBlockCache->loadFromDB();
                }
        }
        
@@ -246,6 +263,10 @@ class Block
                return $this->mNetworkBits;
        }
 
+       function forUpdate( $x = NULL ) {
+               return wfSetVar( $this->mForUpdate, $x );
+       }
+
        /* static */ function getAutoblockExpiry( $timestamp )
        {
                global $wgAutoblockExpiry;

diff --git a/includes/BlockCache.php b/includes/BlockCache.php
index 8b49a01..9f099b2 100644 (file)
--- a/includes/BlockCache.php
+++ b/includes/BlockCache.php
@@ -7,8 +7,7 @@ class BlockCache
 {
        var $mData = false, $mMemcKey;
 
-       function BlockCache( $deferLoad = false, $dbName = '' )
-       {
+       function BlockCache( $deferLoad = false, $dbName = '' ) {
                global $wgDBname;
 
                if ( $dbName == '' ) {
@@ -22,34 +21,38 @@ class BlockCache
                }
        }
 
-       function load()
-       {
+       # Load the blocks from the database and save them to memcached
+       function loadFromDB() {
+               global $wgUseMemCached, $wgMemc;
+               $this->mData = array();
+               # Selecting FOR UPDATE is a convenient way to serialise the memcached and DB operations,
+               # which is necessary even though we don't update the DB
+               if ( $wgUseMemCached ) {
+                       Block::enumBlocks( 'wfBlockCacheInsert', '', EB_FOR_UPDATE );
+                       $wgMemc->set( $this->mMemcKey, $this->mData, 0 );
+               } else {
+                       Block::enumBlocks( 'wfBlockCacheInsert', '' );
+               }
+       }
+               
+       # Load the cache from memcached or, if that's not possible, from the DB
+       function load() {
                global $wgUseMemCached, $wgMemc;
 
                if ( $this->mData === false) {
-                       $saveMemc = false;
                        # Try memcached
                        if ( $wgUseMemCached ) {
                                $this->mData = $wgMemc->get( $this->mMemcKey );
-                               if ( !$this->mData ) {
-                                       $saveMemc = true;
-                               }
                        }
 
                        if ( !is_array( $this->mData ) ) {
-                               # Load from DB
-                               $this->mData = array();
-                               Block::enumBlocks( 'wfBlockCacheInsert', '' ); # Calls $this->insert()
-                       }
-                       
-                       if ( $saveMemc ) {
-                               $wgMemc->set( $this->mMemcKey, $this->mData, 0 );
+                               $this->loadFromDB();
                        }
                }
        }
 
-       function insert( &$block )
-       {
+       # Add a block to the cache
+       function insert( &$block ) {
                if ( $block->mUser == 0 ) {
                        $nb = $block->getNetworkBits();
                        $ipint = $block->getIntegerAddr();
@@ -62,9 +65,9 @@ class BlockCache
                        $this->mData[$nb][$index] = 1;
                }
        }
-               
-       function get( $ip )
-       {
+       
+       # Find out if a given IP address is blocked
+       function get( $ip ) {
                $this->load();
                $ipint = ip2long( $ip );
                $blocked = false;
@@ -90,24 +93,14 @@ class BlockCache
                return $block;
        }
 
-       function clear()
-       {
-               global $wgUseMemCached, $wgMemc;
-
-               $this->mData = false;
-               if ( $wgUseMemCached ) {
-                       $wgMemc->delete( $this->mMemcKey );
-               }
-       }
-
-       function clearLocal()
-       {
+       # Clear the local cache
+       # There was once a clear() to clear memcached too, but I deleted it
+       function clearLocal() {
                $this->mData = false;
        }
 }
 
-function wfBlockCacheInsert( $block, $tag )
-{
+function wfBlockCacheInsert( $block, $tag ) {
        global $wgBlockCache;
        $wgBlockCache->insert( $block );
 }