| Index: trunk/extensions/InterfaceConcurrency/InterfaceConcurrency.php |
| — | — | @@ -68,9 +68,9 @@ |
| 69 | 69 | ); |
| 70 | 70 | |
| 71 | 71 | // Configuration |
| 72 | | -$wgConcurrency = array( |
| | 72 | +$wgInterfaceConcurrencyConfig = array( |
| 73 | 73 | 'ExpirationDefault' => 60 * 15, // Default checkout duration. 15 minutes. |
| 74 | 74 | 'ExpirationMax' => 60 * 30, // Maximum possible checkout duration. 30 minutes. |
| 75 | 75 | 'ExpirationMin' => 1, // Minimum possible checkout duration. Negative is possible for testing if you want. |
| 76 | | - 'TrustMemc' => true // If running in an environment with multiple discrete caches, set to false. |
| | 76 | + 'ListMaxAge' => 60, // How stale (in seconds) can listCheckouts() be? |
| 77 | 77 | ); |
| Index: trunk/extensions/InterfaceConcurrency/tests/phpunit/ConcurrencyCheckTest.php |
| — | — | @@ -28,10 +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, $wgConcurrency; |
| | 32 | + global $wgMainCacheType, $wgInterfaceConcurrencyConfig; |
| 33 | 33 | $this->oldcache = $wgMainCacheType; |
| 34 | 34 | $wgMainCacheType = CACHE_MEMCACHED; |
| 35 | | - $wgConcurrency['ExpirationMin'] = -60; // negative numbers are needed for testing |
| | 35 | + $wgInterfaceConcurrencyConfig['ExpirationMin'] = -60; // negative numbers are needed for testing |
| 36 | 36 | } |
| 37 | 37 | |
| 38 | 38 | public function tearDown() { |
| — | — | @@ -101,4 +101,16 @@ |
| 102 | 102 | $this->assertEquals( 'invalid', $output[1339]['status'], "Expired checkouts are invalid" ); |
| 103 | 103 | $this->assertEquals( 'invalid', $output[13310]['status'], "Missing checkouts are invalid" ); |
| 104 | 104 | } |
| | 105 | + |
| | 106 | + public function testListCheckouts() { |
| | 107 | + $cc = new ConcurrencyCheck( 'CCUnitTest', self::$users['user1']->user ); |
| | 108 | + $cc->checkout( 1337 ); |
| | 109 | + $cc->checkout( 1338 ); |
| | 110 | + |
| | 111 | + $output = $cc->listCheckouts(); |
| | 112 | + $this->assertTrue( $output[1337] && $output[1338], "Current checkouts are present" ); |
| | 113 | + $this->assertEquals( self::$users['user1']->user->getId(), $output[1337]['cc_user'], "User matches" ); |
| | 114 | + $this->assertTrue( array_key_exists( 'cc_expiration', $output[1337] ), "Expiration exists" ); |
| | 115 | + $this->assertTrue( $output[1337]['mine'], "Ownership flag set" ); |
| | 116 | + } |
| 105 | 117 | } |
| Index: trunk/extensions/InterfaceConcurrency/includes/ConcurrencyCheck.php |
| — | — | @@ -59,27 +59,27 @@ |
| 60 | 60 | $userId = $this->user->getId(); |
| 61 | 61 | $cacheKey = wfMemcKey( 'concurrencycheck', $this->resourceType, $record ); |
| 62 | 62 | |
| 63 | | - // when operating with a single memcached cluster, it's reasonable to check the cache here. |
| 64 | | - global $wgConcurrency; |
| 65 | | - if ( $wgConcurrency['TrustMemc'] ) { |
| 66 | | - $cached = $wgMemc->get( $cacheKey ); |
| 67 | | - if ( $cached ) { |
| 68 | | - if ( !$override && $cached['userId'] != $userId && $cached['expiration'] > time() ) { |
| 69 | | - // this is already checked out. |
| 70 | | - return false; |
| 71 | | - } |
| | 63 | + global $wgInterfaceConcurrencyConfig; |
| | 64 | + |
| | 65 | + // check the cache first. |
| | 66 | + $cached = $wgMemc->get( $cacheKey ); |
| | 67 | + if ( $cached ) { |
| | 68 | + if ( !$override && $cached['userId'] != $userId && $cached['expiration'] > time() ) { |
| | 69 | + // this is already checked out. |
| | 70 | + return false; |
| 72 | 71 | } |
| 73 | 72 | } |
| 74 | 73 | |
| 75 | | - // attempt an insert, check success (this is atomic) |
| | 74 | + // attempt an insert, check success |
| 76 | 75 | $insertError = null; |
| | 76 | + $expiration = time() + $this->expirationTime; |
| 77 | 77 | $dbw->insert( |
| 78 | 78 | 'concurrencycheck', |
| 79 | 79 | array( |
| 80 | 80 | 'cc_resource_type' => $this->resourceType, |
| 81 | 81 | 'cc_record' => $record, |
| 82 | 82 | 'cc_user' => $userId, |
| 83 | | - 'cc_expiration' => wfTimestamp( TS_MW, time() + $this->expirationTime ), |
| | 83 | + 'cc_expiration' => $dbw->timestamp( $expiration ), |
| 84 | 84 | ), |
| 85 | 85 | __METHOD__, |
| 86 | 86 | array( 'IGNORE' ) |
| — | — | @@ -87,41 +87,38 @@ |
| 88 | 88 | |
| 89 | 89 | // if the insert succeeded, checkout is done. |
| 90 | 90 | if ( $dbw->affectedRows() === 1 ) { |
| 91 | | - // delete any existing cache key. can't create a new key here |
| 92 | | - // since the insert didn't happen inside a transaction. |
| 93 | | - $wgMemc->delete( $cacheKey ); |
| | 91 | + // "By default, MediaWiki opens a transaction at the first query, and commits it before the output is sent." |
| | 92 | + // set the cache key, since this is inside an implicit transaction. |
| | 93 | + $wgMemc->set( $cacheKey, array( |
| | 94 | + 'userId' => $userId, |
| | 95 | + 'expiration' => $expiration, |
| | 96 | + ), |
| | 97 | + $this->expirationTime |
| | 98 | + ); |
| | 99 | + |
| | 100 | + $dbw->commit( __METHOD__ ); |
| 94 | 101 | return true; |
| 95 | 102 | } |
| 96 | 103 | |
| 97 | | - // if the insert failed, it's necessary to check the expiration. |
| 98 | | - // here, check by deleting, since that permits the use of write locks |
| 99 | | - // (SELECT..LOCK IN SHARE MODE), rather than read locks (SELECT..FOR UPDATE) |
| 100 | | - $dbw->begin( __METHOD__ ); |
| 101 | | - $dbw->delete( |
| | 104 | + // the insert failed, which means there's an existing row. |
| | 105 | + $row = $dbw->selectRow( |
| 102 | 106 | 'concurrencycheck', |
| | 107 | + array( '*' ), |
| 103 | 108 | array( |
| 104 | 109 | 'cc_resource_type' => $this->resourceType, |
| 105 | 110 | 'cc_record' => $record, |
| 106 | | - '(cc_user = ' . $userId . ' OR cc_expiration <= ' . $dbw->addQuotes( wfTimestamp( TS_MW ) ) . ')', // only the owner can perform a checkin |
| 107 | 111 | ), |
| 108 | 112 | __METHOD__, |
| 109 | 113 | array() |
| 110 | 114 | ); |
| 111 | 115 | |
| 112 | | - // delete failed: not checked out by current user, checkout is unexpired, override is unset |
| 113 | | - if ( $dbw->affectedRows() !== 1 && ! $override ) { |
| 114 | | - // fetch the existing data to cache it |
| 115 | | - $row = $dbw->selectRow( |
| 116 | | - 'concurrencycheck', |
| 117 | | - array( '*' ), |
| 118 | | - array( |
| 119 | | - 'cc_resource_type' => $this->resourceType, |
| 120 | | - 'cc_record' => $record, |
| 121 | | - ), |
| 122 | | - __METHOD__, |
| 123 | | - array() |
| 124 | | - ); |
| 125 | | - |
| | 116 | + // checked out by current user, checkout is unexpired, override is unset |
| | 117 | + if ( |
| | 118 | + $row && |
| | 119 | + $row->cc_user != $userId && |
| | 120 | + wfTimestamp( TS_UNIX, $row->cc_expiration ) > time() && |
| | 121 | + ! $override |
| | 122 | + ) { |
| 126 | 123 | // this was a cache miss. populate the cache with data from the db. |
| 127 | 124 | // cache is set to expire at the same time as the checkout, since it'll become invalid then anyway. |
| 128 | 125 | // inside this transaction, a row-level lock is established which ensures cache concurrency |
| — | — | @@ -135,9 +132,6 @@ |
| 136 | 133 | return false; |
| 137 | 134 | } |
| 138 | 135 | |
| 139 | | - $expiration = time() + $this->expirationTime; |
| 140 | | - |
| 141 | | - // delete succeeded, insert a new row. |
| 142 | 136 | // replace is used here to support the override parameter |
| 143 | 137 | $res = $dbw->replace( |
| 144 | 138 | 'concurrencycheck', |
| — | — | @@ -146,7 +140,7 @@ |
| 147 | 141 | 'cc_resource_type' => $this->resourceType, |
| 148 | 142 | 'cc_record' => $record, |
| 149 | 143 | 'cc_user' => $userId, |
| 150 | | - 'cc_expiration' => wfTimestamp( TS_MW, $expiration ), |
| | 144 | + 'cc_expiration' => $dbw->timestamp( $expiration ), |
| 151 | 145 | ), |
| 152 | 146 | __METHOD__ |
| 153 | 147 | ); |
| — | — | @@ -182,12 +176,15 @@ |
| 183 | 177 | array() |
| 184 | 178 | ); |
| 185 | 179 | |
| 186 | | - // check row count (this is atomic, select would not be) |
| | 180 | + // check row count (this is atomic since the dml establishes a lock, select would not be) |
| | 181 | + // not that it matters too much, since cache deletes can happen w/o atomicity |
| 187 | 182 | if ( $dbw->affectedRows() === 1 ) { |
| 188 | 183 | $wgMemc->delete( $cacheKey ); |
| | 184 | + $dbw->commit( __METHOD__ ); |
| 189 | 185 | return true; |
| 190 | 186 | } |
| 191 | 187 | |
| | 188 | + $dbw->commit( __METHOD__ ); |
| 192 | 189 | return false; |
| 193 | 190 | } |
| 194 | 191 | |
| — | — | @@ -199,23 +196,30 @@ |
| 200 | 197 | public function expire() { |
| 201 | 198 | // TODO: run this in a few other places that db access happens, to make sure the db stays non-crufty. |
| 202 | 199 | $dbw = $this->dbw; |
| 203 | | - $now = time(); |
| 204 | 200 | |
| 205 | 201 | // remove the rows from the db. trust memcached to expire the cache. |
| 206 | 202 | $dbw->delete( |
| 207 | 203 | 'concurrencycheck', |
| 208 | 204 | array( |
| 209 | | - 'cc_expiration <= ' . $dbw->addQuotes( wfTimestamp( TS_MW, $now ) ), |
| | 205 | + 'cc_expiration <= ' . $dbw->addQuotes( $dbw->timestamp() ), |
| 210 | 206 | ), |
| 211 | 207 | __METHOD__, |
| 212 | 208 | array() |
| 213 | 209 | ); |
| 214 | 210 | |
| 215 | | - // return the number of rows removed. |
| 216 | | - return $dbw->affectedRows(); |
| | 211 | + $rowCount = $dbw->affectedRows(); |
| | 212 | + $dbw->commit( __METHOD__ ); |
| | 213 | + |
| | 214 | + // if I'm going to assume an implicit transaction, best not to contradict that assumption. |
| | 215 | + // expire() is called internally. |
| | 216 | + $dbw->begin( __METHOD__ ); |
| | 217 | + |
| | 218 | + return $rowCount; |
| 217 | 219 | } |
| 218 | 220 | |
| 219 | 221 | /** |
| | 222 | + * Get the status of a set of known keys |
| | 223 | + * |
| 220 | 224 | * @param $keys array |
| 221 | 225 | * @return array |
| 222 | 226 | */ |
| — | — | @@ -238,7 +242,7 @@ |
| 239 | 243 | 'cc_resource_type' => $this->resourceType, |
| 240 | 244 | 'cc_record' => $key, |
| 241 | 245 | 'cc_user' => $cached['userId'], |
| 242 | | - 'cc_expiration' => wfTimestamp( TS_MW, $cached['expiration'] ), |
| | 246 | + 'cc_expiration' => $dbw->timestamp( $cached['expiration'] ), |
| 243 | 247 | 'cache' => 'cached', |
| 244 | 248 | ); |
| 245 | 249 | } else { |
| — | — | @@ -251,24 +255,21 @@ |
| 252 | 256 | // If it's time to go to the database, go ahead and expire old rows. |
| 253 | 257 | $this->expire(); |
| 254 | 258 | |
| 255 | | - // Why LOCK IN SHARE MODE, you might ask? To avoid a race condition: Otherwise, it's possible for |
| 256 | | - // a checkin and/or checkout to occur between this select and the value being stored in cache, which |
| 257 | | - // makes for an incorrect cache. This, in turn, could make checkout() above (which uses the cache) |
| 258 | | - // function incorrectly. |
| | 259 | + // Why LOCK IN SHARE MODE, you might ask? To avoid a race condition: Otherwise, it's |
| | 260 | + // possible for a checkin and/or checkout to occur between this select and the value |
| | 261 | + // being stored in cache, which makes for an incorrect cache. This, in turn, could |
| | 262 | + // make checkout() above (which uses the cache) function incorrectly. |
| 259 | 263 | // |
| 260 | | - // Another option would be to run the select, then check each row in-turn before setting the cache |
| 261 | | - // key using either SELECT (with LOCK IN SHARE MODE) or UPDATE that checks a timestamp (and which |
| 262 | | - // would establish the same lock). That method would mean smaller, quicker locks, but more overall |
| 263 | | - // database overhead. |
| | 264 | + // With LOCK IN SHARE MODE on, a concurrent INSERT call in checkout() will make this |
| | 265 | + // select wait, and this query will wait for the transaction in checkout(). |
| 264 | 266 | // |
| 265 | | - // It appears all the DBMSes we use support LOCK IN SHARE MODE, but if that's not the case, the second |
| 266 | | - // solution above could be implemented instead. |
| | 267 | + // Another option would be to run the select, then check each row in-turn before |
| | 268 | + // setting the cache key using either SELECT (with LOCK IN SHARE MODE) or UPDATE |
| | 269 | + // that checks a timestamp (and which would establish the same lock). That method |
| | 270 | + // would mean smaller, quicker locks, but more overall database overhead. |
| 267 | 271 | $queryParams = array(); |
| 268 | 272 | if ( $wgDBtype === 'mysql' ) { |
| 269 | 273 | $queryParams[] = 'LOCK IN SHARE MODE'; |
| 270 | | - |
| 271 | | - // the transaction seems incongruous, I know, but it's to keep the cache update atomic. |
| 272 | | - $dbw->begin( __METHOD__ ); |
| 273 | 274 | } |
| 274 | 275 | |
| 275 | 276 | $res = $dbw->select( |
| — | — | @@ -277,7 +278,7 @@ |
| 278 | 279 | array( |
| 279 | 280 | 'cc_resource_type' => $this->resourceType, |
| 280 | 281 | 'cc_record' => $toSelect, |
| 281 | | - 'cc_expiration > ' . $dbw->addQuotes( wfTimestamp( TS_MW ) ), |
| | 282 | + 'cc_expiration > ' . $dbw->addQuotes( $dbw->timestamp() ), |
| 282 | 283 | ), |
| 283 | 284 | __METHOD__, |
| 284 | 285 | $queryParams |
| — | — | @@ -316,11 +317,60 @@ |
| 317 | 318 | return $checkouts; |
| 318 | 319 | } |
| 319 | 320 | |
| | 321 | + /** |
| | 322 | + * Get a list of all the currently checked out records |
| | 323 | + * |
| | 324 | + * This information can be stale by up to $wgInterfaceConcurrencyConfig['ListMaxAge'] |
| | 325 | + * |
| | 326 | + * @return Array describing existing checkouts |
| | 327 | + */ |
| 320 | 328 | public function listCheckouts() { |
| 321 | | - // TODO: fill in the function that lets you get the complete set of checkouts for a given application. |
| | 329 | + global $wgMemc, $wgInterfaceConcurrencyConfig; |
| | 330 | + $dbw = $this->dbw; |
| | 331 | + $userId = $this->user->getId(); |
| | 332 | + $cacheKey = wfMemcKey( 'concurrencycheck', $this->resourceType, 'currentCheckouts' ); |
| | 333 | + |
| | 334 | + $checkouts = $wgMemc->get($cacheKey); |
| | 335 | + |
| | 336 | + // not cached? fetch from the db |
| | 337 | + if( ! $checkouts ) { |
| | 338 | + // when going to the db, remove expired rows. |
| | 339 | + $this->expire(); |
| | 340 | + |
| | 341 | + $res = $dbw->select( |
| | 342 | + 'concurrencycheck', |
| | 343 | + array( '*' ), |
| | 344 | + array( |
| | 345 | + 'cc_resource_type' => $this->resourceType, |
| | 346 | + ), |
| | 347 | + __METHOD__, |
| | 348 | + array() |
| | 349 | + ); |
| | 350 | + |
| | 351 | + while ( $res && $record = $res->fetchRow() ) { |
| | 352 | + $checkouts[$record['cc_record']] = $record; |
| | 353 | + } |
| | 354 | + |
| | 355 | + $wgMemc->set( $cacheKey, $checkouts, $wgInterfaceConcurrencyConfig['ListMaxAge'] ); |
| | 356 | + } |
| | 357 | + |
| | 358 | + $checkoutsReturn = $checkouts; |
| | 359 | + foreach( $checkouts as $record => $values ) { |
| | 360 | + // does this checkout belong to the current user? |
| | 361 | + if( $values['cc_user'] == $userId ) { |
| | 362 | + $checkoutsReturn[$record]['mine'] = true; |
| | 363 | + } else { |
| | 364 | + $checkoutsReturn[$record]['mine'] = false; |
| | 365 | + } |
| | 366 | + } |
| | 367 | + |
| | 368 | + return $checkoutsReturn; |
| | 369 | + |
| 322 | 370 | } |
| 323 | 371 | |
| 324 | 372 | /** |
| | 373 | + * Setter for user. |
| | 374 | + * |
| 325 | 375 | * @param $user user |
| 326 | 376 | */ |
| 327 | 377 | public function setUser( $user ) { |
| — | — | @@ -328,23 +378,25 @@ |
| 329 | 379 | } |
| 330 | 380 | |
| 331 | 381 | /** |
| | 382 | + * Setter for expirationTime |
| | 383 | + * |
| 332 | 384 | * @param $expirationTime int|null |
| 333 | 385 | */ |
| 334 | 386 | public function setExpirationTime( $expirationTime = null ) { |
| 335 | | - global $wgConcurrency; |
| | 387 | + global $wgInterfaceConcurrencyConfig; |
| 336 | 388 | |
| 337 | 389 | // check to make sure the time is a number |
| 338 | 390 | // negative number are allowed, though mostly only used for testing |
| 339 | 391 | if ( $expirationTime && (int) $expirationTime == $expirationTime ) { |
| 340 | | - if ( $expirationTime > $wgConcurrency['ExpirationMax'] ) { |
| 341 | | - $this->expirationTime = $wgConcurrency['ExpirationMax']; // if the number is too high, limit it to the max value. |
| 342 | | - } elseif ( $expirationTime < $wgConcurrency['ExpirationMin'] ) { |
| 343 | | - $this->expirationTime = $wgConcurrency['ExpirationMin']; // low limit, default -1 min |
| | 392 | + if ( $expirationTime > $wgInterfaceConcurrencyConfig['ExpirationMax'] ) { |
| | 393 | + $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationMax']; // if the number is too high, limit it to the max value. |
| | 394 | + } elseif ( $expirationTime < $wgInterfaceConcurrencyConfig['ExpirationMin'] ) { |
| | 395 | + $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationMin']; // low limit, default -1 min |
| 344 | 396 | } else { |
| 345 | 397 | $this->expirationTime = $expirationTime; // the amount of time before a checkout expires. |
| 346 | 398 | } |
| 347 | 399 | } else { |
| 348 | | - $this->expirationTime = $wgConcurrency['ExpirationDefault']; // global default is 15 mins. |
| | 400 | + $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationDefault']; // global default is 15 mins. |
| 349 | 401 | } |
| 350 | 402 | } |
| 351 | 403 | |