| Index: trunk/extensions/InterfaceConcurrency/tests/phpunit/ApiConcurrencyTest.php |
| — | — | @@ -77,7 +77,6 @@ |
| 78 | 78 | global $wgUser; |
| 79 | 79 | |
| 80 | 80 | $wgUser = self::$users['one']->user; |
| 81 | | - /* commenting these out since i need to go home and they're breakin CI. See commit summary for details. |
| 82 | 81 | |
| 83 | 82 | list( $result, , $session ) = $this->doApiRequestWithToken( array( |
| 84 | 83 | 'action' => 'concurrency', |
| — | — | @@ -104,7 +103,6 @@ |
| 105 | 104 | 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['two'], self::$users['two']->user ); |
| 106 | 105 | |
| 107 | 106 | $this->assertEquals( "success", $result['concurrency']['result'] ); |
| 108 | | - */ |
| 109 | 107 | } |
| 110 | 108 | |
| 111 | 109 | /** |
| — | — | @@ -115,7 +113,6 @@ |
| 116 | 114 | global $wgUser; |
| 117 | 115 | |
| 118 | 116 | $wgUser = self::$users['one']->user; |
| 119 | | - /* commenting these out since i need to go home and they're breakin CI. See commit summary for details. |
| 120 | 117 | |
| 121 | 118 | list( $result, , $session ) = $this->doApiRequestWithToken( array( |
| 122 | 119 | 'action' => 'concurrency', |
| — | — | @@ -142,7 +139,6 @@ |
| 143 | 140 | 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['two'], self::$users['two']->user ); |
| 144 | 141 | |
| 145 | 142 | $this->assertEquals( "success", $result['concurrency']['result'] ); |
| 146 | | - */ |
| 147 | 143 | } |
| 148 | 144 | |
| 149 | 145 | /** |
| — | — | @@ -166,7 +162,6 @@ |
| 167 | 163 | $e->getMessage() ); |
| 168 | 164 | } |
| 169 | 165 | $this->assertTrue( $exception, "Got exception" ); |
| 170 | | - |
| 171 | 166 | } |
| 172 | 167 | |
| 173 | 168 | } |
| Index: trunk/extensions/InterfaceConcurrency/tests/phpunit/ConcurrencyCheckTest.php |
| — | — | @@ -31,7 +31,8 @@ |
| 32 | 32 | global $wgMainCacheType, $wgInterfaceConcurrencyConfig; |
| 33 | 33 | $this->oldcache = $wgMainCacheType; |
| 34 | 34 | $wgMainCacheType = CACHE_MEMCACHED; |
| 35 | | - $wgInterfaceConcurrencyConfig['ExpirationMin'] = -60; // negative numbers are needed for testing |
| | 35 | + // negative numbers are needed for testing |
| | 36 | + $wgInterfaceConcurrencyConfig['ExpirationMin'] = -60; |
| 36 | 37 | } |
| 37 | 38 | |
| 38 | 39 | public function tearDown() { |
| — | — | @@ -56,19 +57,43 @@ |
| 57 | 58 | // tests |
| 58 | 59 | $this->assertTrue( $first->checkout( $testKey ), "Initial checkout" ); |
| 59 | 60 | $this->assertTrue( $first->checkout( $testKey ), "Cache hit" ); |
| 60 | | - $this->assertFalse( $second->checkout( $testKey ), "Checkout of locked resource fails as different user" ); |
| 61 | | - $this->assertTrue( $first->checkout( $testKey ), "Checkout of locked resource succeeds as original user" ); |
| 62 | | - $this->assertFalse( $second->checkin( $testKey ), "Checkin of locked resource fails as different user" ); |
| 63 | | - $this->assertTrue( $first->checkin( $testKey ), "Checkin of locked resource succeeds as original user" ); |
| | 61 | + $this->assertFalse( |
| | 62 | + $second->checkout( $testKey ), |
| | 63 | + "Checkout of locked resource fails as different user" |
| | 64 | + ); |
| | 65 | + $this->assertTrue( |
| | 66 | + $first->checkout( $testKey ), |
| | 67 | + "Checkout of locked resource succeeds as original user" |
| | 68 | + ); |
| | 69 | + $this->assertFalse( |
| | 70 | + $second->checkin( $testKey ), |
| | 71 | + "Checkin of locked resource fails as different user" |
| | 72 | + ); |
| | 73 | + $this->assertTrue( |
| | 74 | + $first->checkin( $testKey ), |
| | 75 | + "Checkin of locked resource succeeds as original user" |
| | 76 | + ); |
| 64 | 77 | $second->setExpirationTime( -5 ); |
| 65 | | - $this->assertTrue( $second->checkout( $testKey ), "Checked-in resource is now available to second user" ); |
| | 78 | + $this->assertTrue( |
| | 79 | + $second->checkout( $testKey ), |
| | 80 | + "Checked-in resource is now available to second user" |
| | 81 | + ); |
| 66 | 82 | $second->setExpirationTime(); |
| 67 | | - $this->assertTrue( $first->checkout( $testKey ), "Checkout of expired resource succeeds as first user" ); |
| | 83 | + $this->assertTrue( |
| | 84 | + $first->checkout( $testKey ), |
| | 85 | + "Checkout of expired resource succeeds as first user" |
| | 86 | + ); |
| 68 | 87 | $this->assertTrue( $second->checkout( $testKey, true ), "Checkout override" ); |
| 69 | | - $this->assertFalse( $first->checkout( $testKey ), "Checkout of overriden resource fails as different user" ); |
| | 88 | + $this->assertFalse( |
| | 89 | + $first->checkout( $testKey ), |
| | 90 | + "Checkout of overriden resource fails as different user" |
| | 91 | + ); |
| 70 | 92 | |
| 71 | 93 | // cleanup |
| 72 | | - $this->assertTrue( $second->checkin( $testKey ), "Checkin of record with changed ownership" ); |
| | 94 | + $this->assertTrue( |
| | 95 | + $second->checkin( $testKey ), |
| | 96 | + "Checkin of record with changed ownership" |
| | 97 | + ); |
| 73 | 98 | |
| 74 | 99 | } |
| 75 | 100 | |
| — | — | @@ -97,9 +122,21 @@ |
| 98 | 123 | $output = $cc->status( array( 1337, 1338, 1339, 13310 ) ); |
| 99 | 124 | $this->assertEquals( true, is_array( $output ), "Status returns values" ); |
| 100 | 125 | $this->assertEquals( 4, count( $output ), "Output has the correct number of records" ); |
| 101 | | - $this->assertEquals( 'valid', $output[1337]['status'], "Current checkouts are listed as valid" ); |
| 102 | | - $this->assertEquals( 'invalid', $output[1339]['status'], "Expired checkouts are invalid" ); |
| 103 | | - $this->assertEquals( 'invalid', $output[13310]['status'], "Missing checkouts are invalid" ); |
| | 126 | + $this->assertEquals( |
| | 127 | + 'valid', |
| | 128 | + $output[1337]['status'], |
| | 129 | + "Current checkouts are listed as valid" |
| | 130 | + ); |
| | 131 | + $this->assertEquals( |
| | 132 | + 'invalid', |
| | 133 | + $output[1339]['status'], |
| | 134 | + "Expired checkouts are invalid" |
| | 135 | + ); |
| | 136 | + $this->assertEquals( |
| | 137 | + 'invalid', |
| | 138 | + $output[13310]['status'], |
| | 139 | + "Missing checkouts are invalid" |
| | 140 | + ); |
| 104 | 141 | } |
| 105 | 142 | |
| 106 | 143 | public function testListCheckouts() { |
| — | — | @@ -109,8 +146,15 @@ |
| 110 | 147 | |
| 111 | 148 | $output = $cc->listCheckouts(); |
| 112 | 149 | $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" ); |
| | 150 | + $this->assertEquals( |
| | 151 | + self::$users['user1']->user->getId(), |
| | 152 | + $output[1337]['cc_user'], |
| | 153 | + "User matches" |
| | 154 | + ); |
| | 155 | + $this->assertTrue( |
| | 156 | + array_key_exists( 'cc_expiration', $output[1337] ), |
| | 157 | + "Expiration exists" |
| | 158 | + ); |
| 115 | 159 | $this->assertTrue( $output[1337]['mine'], "Ownership flag set" ); |
| 116 | 160 | } |
| 117 | 161 | } |
| Index: trunk/extensions/InterfaceConcurrency/includes/ConcurrencyCheck.php |
| — | — | @@ -3,17 +3,18 @@ |
| 4 | 4 | /** |
| 5 | 5 | * Class for cooperative locking of web resources |
| 6 | 6 | * |
| 7 | | - * Each resource is identified by a combination of the "resource type" (the application, the type |
| 8 | | - * of content, etc), and the resource's primary key or some other unique numeric ID. |
| | 7 | + * Each resource is identified by a combination of the "resource type" (the application, the |
| | 8 | + * type of content, etc), and the resource's primary key or some other unique numeric ID. |
| 9 | 9 | * |
| 10 | | - * Currently, a resource can only be checked out by a single user. Other attempts to check it out result |
| 11 | | - * in the checkout failing. In the future, an option for multiple simulataneous checkouts could be added |
| 12 | | - * without much trouble. |
| | 10 | + * Currently, a resource can only be checked out by a single user. Other attempts to check it |
| | 11 | + * out result in the checkout failing. In the future, an option for multiple simulataneous |
| | 12 | + * checkouts could be added without much trouble. |
| 13 | 13 | * |
| 14 | | - * This could be done with named locks, except then it would be impossible to build a list of all the |
| 15 | | - * resources currently checked out for a given application. There's no good way to construct a query |
| 16 | | - * that answers the question, "What locks do you have starting with [foo]" This could be done really well |
| 17 | | - * with a concurrent, reliable, distributed key/value store, but we don't have one of those right now. |
| | 14 | + * This could be done with named locks, except then it would be impossible to build a list of |
| | 15 | + * all the resources currently checked out for a given application. There's no good way to |
| | 16 | + * construct a query that answers the question, "What locks do you have starting with [foo]" |
| | 17 | + * This could be done really well with a concurrent, reliable, distributed key/value store, |
| | 18 | + * but we don't have one of those right now. |
| 18 | 19 | * |
| 19 | 20 | * @author Ian Baker <ian@wikimedia.org> |
| 20 | 21 | */ |
| — | — | @@ -34,8 +35,8 @@ |
| 35 | 36 | */ |
| 36 | 37 | public function __construct( $resourceType, $user, $expirationTime = null ) { |
| 37 | 38 | |
| 38 | | - // All database calls are to the master, since the whole point of this class is maintaining |
| 39 | | - // concurrency. Most reads should come from cache anyway. |
| | 39 | + // All database calls are to the master, since the whole point of this class is |
| | 40 | + // maintaining concurrency. Most reads should come from cache anyway. |
| 40 | 41 | $this->dbw = wfGetDb( DB_MASTER ); |
| 41 | 42 | |
| 42 | 43 | $this->user = $user; |
| — | — | @@ -64,7 +65,11 @@ |
| 65 | 66 | // check the cache first. |
| 66 | 67 | $cached = $wgMemc->get( $cacheKey ); |
| 67 | 68 | if ( $cached ) { |
| 68 | | - if ( !$override && $cached['userId'] != $userId && $cached['expiration'] > time() ) { |
| | 69 | + if ( |
| | 70 | + !$override && |
| | 71 | + $cached['userId'] != $userId && |
| | 72 | + $cached['expiration'] > time() |
| | 73 | + ) { |
| 69 | 74 | // this is already checked out. |
| 70 | 75 | return false; |
| 71 | 76 | } |
| — | — | @@ -87,7 +92,8 @@ |
| 88 | 93 | |
| 89 | 94 | // if the insert succeeded, checkout is done. |
| 90 | 95 | if ( $dbw->affectedRows() === 1 ) { |
| 91 | | - // "By default, MediaWiki opens a transaction at the first query, and commits it before the output is sent." |
| | 96 | + // "By default, MediaWiki opens a transaction at the first query, and commits |
| | 97 | + // it before the output is sent." |
| 92 | 98 | // set the cache key, since this is inside an implicit transaction. |
| 93 | 99 | $wgMemc->set( $cacheKey, array( |
| 94 | 100 | 'userId' => $userId, |
| — | — | @@ -120,8 +126,10 @@ |
| 121 | 127 | ! $override |
| 122 | 128 | ) { |
| 123 | 129 | // this was a cache miss. populate the cache with data from the db. |
| 124 | | - // cache is set to expire at the same time as the checkout, since it'll become invalid then anyway. |
| 125 | | - // inside this transaction, a row-level lock is established which ensures cache concurrency |
| | 130 | + // cache is set to expire at the same time as the checkout, since it'll become |
| | 131 | + // invalid then anyway. |
| | 132 | + // inside this transaction, a row-level lock is established which ensures cache |
| | 133 | + // concurrency |
| 126 | 134 | $wgMemc->set( $cacheKey, array( |
| 127 | 135 | 'userId' => $row->cc_user, |
| 128 | 136 | 'expiration' => wfTimestamp( TS_UNIX, $row->cc_expiration ) |
| — | — | @@ -146,7 +154,11 @@ |
| 147 | 155 | ); |
| 148 | 156 | |
| 149 | 157 | // cache the result. |
| 150 | | - $wgMemc->set( $cacheKey, array( 'userId' => $userId, 'expiration' => $expiration ), $this->expirationTime ); |
| | 158 | + $wgMemc->set( |
| | 159 | + $cacheKey, |
| | 160 | + array( 'userId' => $userId, 'expiration' => $expiration ), |
| | 161 | + $this->expirationTime |
| | 162 | + ); |
| 151 | 163 | |
| 152 | 164 | $dbw->commit( __METHOD__ ); |
| 153 | 165 | return true; |
| — | — | @@ -176,8 +188,8 @@ |
| 177 | 189 | array() |
| 178 | 190 | ); |
| 179 | 191 | |
| 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 |
| | 192 | + // check row count (this is atomic since the dml establishes a lock, select would |
| | 193 | + // not be). not that it matters too much, since cache deletes can happen w/o atomicity |
| 182 | 194 | if ( $dbw->affectedRows() === 1 ) { |
| 183 | 195 | $wgMemc->delete( $cacheKey ); |
| 184 | 196 | $dbw->commit( __METHOD__ ); |
| — | — | @@ -194,7 +206,8 @@ |
| 195 | 207 | * @return Integer describing the number of records expired. |
| 196 | 208 | */ |
| 197 | 209 | public function expire() { |
| 198 | | - // TODO: run this in a few other places that db access happens, to make sure the db stays non-crufty. |
| | 210 | + // TODO: run this in a few other places that db access happens, to make sure the db |
| | 211 | + // stays non-crufty. |
| 199 | 212 | $dbw = $this->dbw; |
| 200 | 213 | |
| 201 | 214 | // remove the rows from the db. trust memcached to expire the cache. |
| — | — | @@ -210,7 +223,8 @@ |
| 211 | 224 | $rowCount = $dbw->affectedRows(); |
| 212 | 225 | $dbw->commit( __METHOD__ ); |
| 213 | 226 | |
| 214 | | - // if I'm going to assume an implicit transaction, best not to contradict that assumption. |
| | 227 | + // if I'm going to assume an implicit transaction, best not to contradict that |
| | 228 | + // assumption. |
| 215 | 229 | // expire() is called internally. |
| 216 | 230 | $dbw->begin( __METHOD__ ); |
| 217 | 231 | |
| — | — | @@ -289,12 +303,16 @@ |
| 290 | 304 | $checkouts[ $record['cc_record'] ] = $record; |
| 291 | 305 | |
| 292 | 306 | // TODO: implement strategy #2 above, determine which DBMSes need which method. |
| 293 | | - // for now, disable adding to cache here for databases that don't support read locking |
| | 307 | + // for now, disable adding to cache here for databases that don't support read |
| | 308 | + // locking |
| 294 | 309 | if ( $wgDBtype !== 'mysql' ) { |
| 295 | 310 | // safe to store values since this is inside the transaction |
| 296 | 311 | $wgMemc->set( |
| 297 | 312 | wfMemcKey( 'concurrencycheck', $this->resourceType, $record['cc_record'] ), |
| 298 | | - array( 'userId' => $record['cc_user'], 'expiration' => wfTimestamp( TS_UNIX, $record['cc_expiration'] ) ), |
| | 313 | + array( |
| | 314 | + 'userId' => $record['cc_user'], |
| | 315 | + 'expiration' => wfTimestamp( TS_UNIX, $record['cc_expiration'] ) |
| | 316 | + ), |
| 299 | 317 | wfTimestamp( TS_UNIX, $record['cc_expiration'] ) - time() |
| 300 | 318 | ); |
| 301 | 319 | } |
| — | — | @@ -389,14 +407,18 @@ |
| 390 | 408 | // negative number are allowed, though mostly only used for testing |
| 391 | 409 | if ( $expirationTime && (int) $expirationTime == $expirationTime ) { |
| 392 | 410 | if ( $expirationTime > $wgInterfaceConcurrencyConfig['ExpirationMax'] ) { |
| 393 | | - $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationMax']; // if the number is too high, limit it to the max value. |
| | 411 | + // if the number is too high, limit it to the max value. |
| | 412 | + $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationMax']; |
| 394 | 413 | } elseif ( $expirationTime < $wgInterfaceConcurrencyConfig['ExpirationMin'] ) { |
| 395 | | - $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationMin']; // low limit, default -1 min |
| | 414 | + // low limit, default -1 min |
| | 415 | + $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationMin']; |
| 396 | 416 | } else { |
| 397 | | - $this->expirationTime = $expirationTime; // the amount of time before a checkout expires. |
| | 417 | + // the amount of time before a checkout expires. |
| | 418 | + $this->expirationTime = $expirationTime; |
| 398 | 419 | } |
| 399 | 420 | } else { |
| 400 | | - $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationDefault']; // global default is 15 mins. |
| | 421 | + // global default is generally 15 mins. |
| | 422 | + $this->expirationTime = $wgInterfaceConcurrencyConfig['ExpirationDefault']; |
| 401 | 423 | } |
| 402 | 424 | } |
| 403 | 425 | |
| — | — | @@ -409,7 +431,9 @@ |
| 410 | 432 | */ |
| 411 | 433 | private static function validateId ( $record ) { |
| 412 | 434 | if ( (int) $record !== $record || $record <= 0 ) { |
| 413 | | - throw new ConcurrencyCheckBadRecordIdException( 'Record ID ' . $record . ' must be a positive integer' ); |
| | 435 | + throw new ConcurrencyCheckBadRecordIdException( |
| | 436 | + 'Record ID ' . $record . ' must be a positive integer' |
| | 437 | + ); |
| 414 | 438 | } |
| 415 | 439 | |
| 416 | 440 | // TODO: add a hook here for client-side validation. |