| Index: trunk/phase3/tests/phpunit/includes/ConcurrencyCheckTest.php |
| — | — | @@ -28,9 +28,10 @@ |
| 29 | 29 | |
| 30 | 30 | // turn on memcached for this test. |
| 31 | 31 | // if no memcached is present, this still works fine. |
| 32 | | - global $wgMainCacheType; |
| | 32 | + global $wgMainCacheType, $wgConcurrency; |
| 33 | 33 | $this->oldcache = $wgMainCacheType; |
| 34 | 34 | $wgMainCacheType = CACHE_MEMCACHED; |
| | 35 | + $wgConcurrency['ExpirationMin'] = -60; // negative numbers are needed for testing |
| 35 | 36 | } |
| 36 | 37 | |
| 37 | 38 | public function tearDown() { |
| Index: trunk/phase3/includes/ConcurrencyCheck.php |
| — | — | @@ -54,16 +54,16 @@ |
| 55 | 55 | * @return boolean |
| 56 | 56 | */ |
| 57 | 57 | public function checkout( $record, $override = null ) { |
| 58 | | - $memc = wfGetMainCache(); |
| | 58 | + global $wgMemc; |
| 59 | 59 | $this->validateId( $record ); |
| 60 | 60 | $dbw = $this->dbw; |
| 61 | 61 | $userId = $this->user->getId(); |
| 62 | | - $cacheKey = wfMemcKey( $this->resourceType, $record ); |
| | 62 | + $cacheKey = wfMemcKey( 'concurrencycheck', $this->resourceType, $record ); |
| 63 | 63 | |
| 64 | 64 | // when operating with a single memcached cluster, it's reasonable to check the cache here. |
| 65 | | - global $wgConcurrencyTrustMemc; |
| 66 | | - if( $wgConcurrencyTrustMemc ) { |
| 67 | | - $cached = $memc->get( $cacheKey ); |
| | 65 | + global $wgConcurrency; |
| | 66 | + if( $wgConcurrency['TrustMemc'] ) { |
| | 67 | + $cached = $wgMemc->get( $cacheKey ); |
| 68 | 68 | if( $cached ) { |
| 69 | 69 | if( ! $override && $cached['userId'] != $userId && $cached['expiration'] > time() ) { |
| 70 | 70 | // this is already checked out. |
| — | — | @@ -90,39 +90,54 @@ |
| 91 | 91 | if( $dbw->affectedRows() === 1 ) { |
| 92 | 92 | // delete any existing cache key. can't create a new key here |
| 93 | 93 | // since the insert didn't happen inside a transaction. |
| 94 | | - $memc->delete( $cacheKey ); |
| | 94 | + $wgMemc->delete( $cacheKey ); |
| 95 | 95 | return true; |
| 96 | 96 | } |
| 97 | 97 | |
| 98 | 98 | // if the insert failed, it's necessary to check the expiration. |
| | 99 | + // here, check by deleting, since that permits the use of write locks |
| | 100 | + // (SELECT..LOCK IN SHARE MODE), rather than read locks (SELECT..FOR UPDATE) |
| 99 | 101 | $dbw->begin(); |
| 100 | | - $row = $dbw->selectRow( |
| | 102 | + $dbw->delete( |
| 101 | 103 | 'concurrencycheck', |
| 102 | | - array( 'cc_user', 'cc_expiration' ), |
| 103 | 104 | array( |
| 104 | 105 | 'cc_resource_type' => $this->resourceType, |
| 105 | 106 | 'cc_record' => $record, |
| | 107 | + '(cc_user = ' . $userId . ' OR cc_expiration <= ' . wfTimestamp( TS_MW ) . ')', // only the owner can perform a checkin |
| 106 | 108 | ), |
| 107 | 109 | __METHOD__, |
| 108 | 110 | array() |
| 109 | 111 | ); |
| 110 | | - |
| 111 | | - // not checked out by current user, checkout is unexpired, override is unset |
| 112 | | - if( ! ( $override || $row->cc_user == $userId || wfTimestamp( TS_UNIX, $row->cc_expiration ) <= time() ) ) { |
| | 112 | + |
| | 113 | + // delete failed: not checked out by current user, checkout is unexpired, override is unset |
| | 114 | + if( $dbw->affectedRows() !== 1 && ! $override) { |
| | 115 | + // fetch the existing data to cache it |
| | 116 | + $row = $dbw->selectRow( |
| | 117 | + 'concurrencycheck', |
| | 118 | + array( '*' ), |
| | 119 | + array( |
| | 120 | + 'cc_resource_type' => $this->resourceType, |
| | 121 | + 'cc_record' => $record, |
| | 122 | + ), |
| | 123 | + __METHOD__, |
| | 124 | + array() |
| | 125 | + ); |
| | 126 | + |
| 113 | 127 | // this was a cache miss. populate the cache with data from the db. |
| 114 | 128 | // cache is set to expire at the same time as the checkout, since it'll become invalid then anyway. |
| 115 | 129 | // inside this transaction, a row-level lock is established which ensures cache concurrency |
| 116 | | - $memc->set( $cacheKey, array( 'userId' => $row->cc_user, 'expiration' => $row->cc_expiration ), wfTimestamp( TS_UNIX, $row->cc_expiration ) - time() ); |
| | 130 | + $wgMemc->set( $cacheKey, array( 'userId' => $row->cc_user, 'expiration' => $row->cc_expiration ), wfTimestamp( TS_UNIX, $row->cc_expiration ) - time() ); |
| 117 | 131 | $dbw->rollback(); |
| 118 | 132 | return false; |
| 119 | 133 | } |
| 120 | 134 | |
| 121 | 135 | $expiration = time() + $this->expirationTime; |
| 122 | 136 | |
| 123 | | - // execute a replace |
| | 137 | + // delete succeeded, insert a new row. |
| | 138 | + // replace is used here to support the override parameter |
| 124 | 139 | $res = $dbw->replace( |
| 125 | 140 | 'concurrencycheck', |
| 126 | | - array( array('cc_resource_type', 'cc_record') ), |
| | 141 | + array( 'cc_resource_type', 'cc_record' ), |
| 127 | 142 | array( |
| 128 | 143 | 'cc_resource_type' => $this->resourceType, |
| 129 | 144 | 'cc_record' => $record, |
| — | — | @@ -133,7 +148,7 @@ |
| 134 | 149 | ); |
| 135 | 150 | |
| 136 | 151 | // cache the result. |
| 137 | | - $memc->set( $cacheKey, array( 'userId' => $userId, 'expiration' => $expiration ), $this->expirationTime ); |
| | 152 | + $wgMemc->set( $cacheKey, array( 'userId' => $userId, 'expiration' => $expiration ), $this->expirationTime ); |
| 138 | 153 | |
| 139 | 154 | $dbw->commit(); |
| 140 | 155 | return true; |
| — | — | @@ -146,11 +161,11 @@ |
| 147 | 162 | * @return Boolean |
| 148 | 163 | */ |
| 149 | 164 | public function checkin( $record ) { |
| 150 | | - $memc = wfGetMainCache(); |
| | 165 | + global $wgMemc; |
| 151 | 166 | $this->validateId( $record ); |
| 152 | 167 | $dbw = $this->dbw; |
| 153 | 168 | $userId = $this->user->getId(); |
| 154 | | - $cacheKey = wfMemcKey( $this->resourceType, $record ); |
| | 169 | + $cacheKey = wfMemcKey( 'concurrencycheck', $this->resourceType, $record ); |
| 155 | 170 | |
| 156 | 171 | $dbw->delete( |
| 157 | 172 | 'concurrencycheck', |
| — | — | @@ -165,7 +180,7 @@ |
| 166 | 181 | |
| 167 | 182 | // check row count (this is atomic, select would not be) |
| 168 | 183 | if( $dbw->affectedRows() === 1 ) { |
| 169 | | - $memc->delete( $cacheKey ); |
| | 184 | + $wgMemc->delete( $cacheKey ); |
| 170 | 185 | return true; |
| 171 | 186 | } |
| 172 | 187 | |
| — | — | @@ -178,28 +193,11 @@ |
| 179 | 194 | * @return Integer describing the number of records expired. |
| 180 | 195 | */ |
| 181 | 196 | public function expire() { |
| 182 | | - $memc = wfGetMainCache(); |
| | 197 | + // TODO: run this in a few other places that db access happens, to make sure the db stays non-crufty. |
| 183 | 198 | $dbw = $this->dbw; |
| 184 | 199 | $now = time(); |
| 185 | 200 | |
| 186 | | - // get the rows to remove from cache. |
| 187 | | - $res = $dbw->select( |
| 188 | | - 'concurrencycheck', |
| 189 | | - array( '*' ), |
| 190 | | - array( |
| 191 | | - 'cc_expiration <= ' . $dbw->addQuotes( wfTimestamp( TS_MW, $now ) ), |
| 192 | | - ), |
| 193 | | - __METHOD__, |
| 194 | | - array() |
| 195 | | - ); |
| 196 | | - |
| 197 | | - // build a list of rows to delete. |
| 198 | | - $toExpire = array(); |
| 199 | | - while( $res && $record = $res->fetchRow() ) { |
| 200 | | - $toExpire[] = $record['cc_record']; |
| 201 | | - } |
| 202 | | - |
| 203 | | - // remove the rows from the db |
| | 201 | + // remove the rows from the db. trust memcached to expire the cache. |
| 204 | 202 | $dbw->delete( |
| 205 | 203 | 'concurrencycheck', |
| 206 | 204 | array( |
| — | — | @@ -209,18 +207,12 @@ |
| 210 | 208 | array() |
| 211 | 209 | ); |
| 212 | 210 | |
| 213 | | - // delete all those rows from cache |
| 214 | | - // outside a transaction because deletes don't require atomicity. |
| 215 | | - foreach( $toExpire as $expire ) { |
| 216 | | - $memc->delete( wfMemcKey( $this->resourceType, $expire ) ); |
| 217 | | - } |
| 218 | | - |
| 219 | 211 | // return the number of rows removed. |
| 220 | 212 | return $dbw->affectedRows(); |
| 221 | 213 | } |
| 222 | 214 | |
| 223 | 215 | public function status( $keys ) { |
| 224 | | - $memc = wfGetMainCache(); |
| | 216 | + global $wgMemc; |
| 225 | 217 | $dbw = $this->dbw; |
| 226 | 218 | $now = time(); |
| 227 | 219 | |
| — | — | @@ -231,7 +223,7 @@ |
| 232 | 224 | foreach( $keys as $key ) { |
| 233 | 225 | $this->validateId( $key ); |
| 234 | 226 | |
| 235 | | - $cached = $memc->get( wfMemcKey( $this->resourceType, $key ) ); |
| | 227 | + $cached = $wgMemc->get( wfMemcKey( 'concurrencycheck', $this->resourceType, $key ) ); |
| 236 | 228 | if( $cached && $cached['expiration'] > $now ) { |
| 237 | 229 | $checkouts[$key] = array( |
| 238 | 230 | 'status' => 'valid', |
| — | — | @@ -253,16 +245,29 @@ |
| 254 | 246 | |
| 255 | 247 | // the transaction seems incongruous, I know, but it's to keep the cache update atomic. |
| 256 | 248 | $dbw->begin(); |
| | 249 | + |
| | 250 | + // Why LOCK IN SHARE MODE, you might ask? To avoid a race condition: Otherwise, it's possible for |
| | 251 | + // a checkin and/or checkout to occur between this select and the value being stored in cache, which |
| | 252 | + // makes for an incorrect cache. This, in turn, could make checkout() above (which uses the cache) |
| | 253 | + // function incorrectly. |
| | 254 | + // |
| | 255 | + // Another option would be to run the select, then check each row in-turn before setting the cache |
| | 256 | + // key using either SELECT (with LOCK IN SHARE MODE) or UPDATE that checks a timestamp (and which |
| | 257 | + // would establish the same lock). That method would mean smaller, quicker locks, but more overall |
| | 258 | + // database overhead. |
| | 259 | + // |
| | 260 | + // It appears all the DBMSes we use support LOCK IN SHARE MODE, but if that's not the case, the second |
| | 261 | + // solution above could be implemented instead. |
| 257 | 262 | $res = $dbw->select( |
| 258 | 263 | 'concurrencycheck', |
| 259 | 264 | array( '*' ), |
| 260 | 265 | array( |
| 261 | 266 | 'cc_resource_type' => $this->resourceType, |
| 262 | | - 'cc_record IN (' . implode( ',', $toSelect ) . ')', |
| | 267 | + 'cc_record' => $toSelect, |
| 263 | 268 | 'cc_expiration > ' . $dbw->addQuotes( wfTimestamp( TS_MW ) ), |
| 264 | 269 | ), |
| 265 | 270 | __METHOD__, |
| 266 | | - array() |
| | 271 | + array('LOCK IN SHARE MODE') |
| 267 | 272 | ); |
| 268 | 273 | |
| 269 | 274 | while( $res && $record = $res->fetchRow() ) { |
| — | — | @@ -270,8 +275,8 @@ |
| 271 | 276 | $checkouts[ $record['cc_record'] ] = $record; |
| 272 | 277 | |
| 273 | 278 | // safe to store values since this is inside the transaction |
| 274 | | - $memc->set( |
| 275 | | - wfMemcKey( $this->resourceType, $record['cc_record'] ), |
| | 279 | + $wgMemc->set( |
| | 280 | + wfMemcKey( 'concurrencycheck', $this->resourceType, $record['cc_record'] ), |
| 276 | 281 | array( 'userId' => $record['cc_user'], 'expiration' => $record['cc_expiration'] ), |
| 277 | 282 | $record['cc_expiration'] - time() |
| 278 | 283 | ); |
| — | — | @@ -304,20 +309,20 @@ |
| 305 | 310 | } |
| 306 | 311 | |
| 307 | 312 | public function setExpirationTime( $expirationTime = null ) { |
| 308 | | - global $wgConcurrencyExpirationDefault, $wgConcurrencyExpirationMax, $wgConcurrencyExpirationMin; |
| 309 | | - |
| 310 | | - // check to make sure the time is digits only, so it can be used in queries |
| | 313 | + global $wgConcurrency; |
| | 314 | + |
| | 315 | + // check to make sure the time is a number |
| 311 | 316 | // negative number are allowed, though mostly only used for testing |
| 312 | | - if( $expirationTime && preg_match('/^[\d-]+$/', $expirationTime) ) { |
| 313 | | - if( $expirationTime > $wgConcurrencyExpirationMax ) { |
| 314 | | - $this->expirationTime = $wgConcurrencyExpirationMax; // if the number is too high, limit it to the max value. |
| 315 | | - } elseif ( $expirationTime < $wgConcurrencyExpirationMin ) { |
| 316 | | - $this->expirationTime = $wgConcurrencyExpirationMin; // low limit, default -1 min |
| | 317 | + if( $expirationTime && (int) $expirationTime == $expirationTime ) { |
| | 318 | + if( $expirationTime > $wgConcurrency['ExpirationMax'] ) { |
| | 319 | + $this->expirationTime = $wgConcurrency['ExpirationMax']; // if the number is too high, limit it to the max value. |
| | 320 | + } elseif ( $expirationTime < $wgConcurrency['ExpirationMin'] ) { |
| | 321 | + $this->expirationTime = $wgConcurrency['ExpirationMin']; // low limit, default -1 min |
| 317 | 322 | } else { |
| 318 | 323 | $this->expirationTime = $expirationTime; // the amount of time before a checkout expires. |
| 319 | 324 | } |
| 320 | 325 | } else { |
| 321 | | - $this->expirationTime = $wgConcurrencyExpirationDefault; // global default is 15 mins. |
| | 326 | + $this->expirationTime = $wgConcurrency['ExpirationDefault']; // global default is 15 mins. |
| 322 | 327 | } |
| 323 | 328 | } |
| 324 | 329 | |
| — | — | @@ -329,7 +334,7 @@ |
| 330 | 335 | * @return boolean |
| 331 | 336 | */ |
| 332 | 337 | private static function validateId ( $record ) { |
| 333 | | - if( ! preg_match('/^\d+$/', $record) ) { |
| | 338 | + if( (int) $record !== $record || $record <= 0 ) { |
| 334 | 339 | throw new ConcurrencyCheckBadRecordIdException( 'Record ID ' . $record . ' must be a positive integer' ); |
| 335 | 340 | } |
| 336 | 341 | |
| Index: trunk/phase3/includes/DefaultSettings.php |
| — | — | @@ -5719,10 +5719,11 @@ |
| 5720 | 5720 | /** |
| 5721 | 5721 | * ConcurrencyCheck keeps track of which web resources are in use, for producing higher-quality UI |
| 5722 | 5722 | */ |
| 5723 | | -$wgConcurrencyExpirationDefault = 60 * 15; // Default checkout duration. 15 minutes. |
| 5724 | | -$wgConcurrencyExpirationMax = 60 * 30; // Maximum possible checkout duration. 30 minutes. |
| 5725 | | -$wgConcurrencyExpirationMin = 60 * -1; // Minimum possible checkout duration. Negative is possible (but barely) for testing. |
| 5726 | | -$wgConcurrencyTrustMemc = true; // If running in an environment with multiple discrete caches, set to false. |
| | 5723 | +$wgConcurrency = array(); |
| | 5724 | +$wgConcurrency['ExpirationDefault'] = 60 * 15; // Default checkout duration. 15 minutes. |
| | 5725 | +$wgConcurrency['ExpirationMax'] = 60 * 30; // Maximum possible checkout duration. 30 minutes. |
| | 5726 | +$wgConcurrency['ExpirationMin'] = 1; // Minimum possible checkout duration. Negative is possible for testing if you want. |
| | 5727 | +$wgConcurrency['TrustMemc'] = true; // If running in an environment with multiple discrete caches, set to false. |
| 5727 | 5728 | |
| 5728 | 5729 | |
| 5729 | 5730 | /** |