r108559 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108558‎ | r108559 | r108560 >
Date:23:03, 10 January 2012
Author:raindrift
Status:reverted (Comments)
Tags:
Comment:
MERGE branches/concurrency 108301:108557 into trunk
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/ConcurrencyCheck.php (added) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/api/ApiConcurrency.php (added) (history)
  • /trunk/phase3/includes/api/ApiMain.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history)
  • /trunk/phase3/maintenance/archives/patch-concurrencycheck.sql (added) (history)
  • /trunk/phase3/maintenance/tables.sql (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/ConcurrencyCheckTest.php (added) (history)
  • /trunk/phase3/tests/phpunit/includes/api/ApiConcurrencyTest.php (added) (history)

Diff [purge]

Index: trunk/phase3/maintenance/archives/patch-concurrencycheck.sql
——@@ -0,0 +1,25 @@
 2+--
 3+-- Store atomic locking information for web resources, to permit
 4+-- UI that warns users when concurrently editing things that aren't
 5+-- concurrently editable.
 6+--
 7+CREATE TABLE /*_*/concurrencycheck (
 8+ -- a string describing the resource or application being checked out.
 9+ cc_resource_type varchar(255) NOT NULL,
 10+
 11+ -- the (numeric) ID of the record that's being checked out.
 12+ cc_record int unsigned NOT NULL,
 13+
 14+ -- the user who has control of the resource
 15+ cc_user int unsigned NOT NULL,
 16+
 17+ -- the date/time on which this record expires
 18+ cc_expiration varbinary(14) not null
 19+
 20+) /*$wgDBTableOptions*/;
 21+-- composite pk.
 22+CREATE UNIQUE INDEX /*i*/cc_resource_record ON /*_*/concurrencycheck (cc_resource_type, cc_record);
 23+-- sometimes there's a delete based on userid.
 24+CREATE INDEX /*i*/cc_user ON /*_*/concurrencycheck (cc_user);
 25+-- sometimes there's a delete based on expiration
 26+CREATE INDEX /*i*/cc_expiration ON /*_*/concurrencycheck (cc_expiration);
Property changes on: trunk/phase3/maintenance/archives/patch-concurrencycheck.sql
___________________________________________________________________
Added: svn:eol-style
127 + native
Index: trunk/phase3/maintenance/tables.sql
——@@ -1483,4 +1483,31 @@
14841484 -- Should cover *most* configuration - strings, ints, bools, etc.
14851485 CREATE INDEX /*i*/cf_name_value ON /*_*/config (cf_name,cf_value(255));
14861486
 1487+--
 1488+-- Store atomic locking information for web resources, to permit
 1489+-- UI that warns users when concurrently editing things that aren't
 1490+-- concurrently editable.
 1491+--
 1492+CREATE TABLE /*_*/concurrencycheck (
 1493+ -- a string describing the resource or application being checked out.
 1494+ cc_resource_type varchar(255) NOT NULL,
 1495+
 1496+ -- the (numeric) ID of the record that's being checked out.
 1497+ cc_record int unsigned NOT NULL,
 1498+
 1499+ -- the user who has control of the resource
 1500+ cc_user int unsigned NOT NULL,
 1501+
 1502+ -- the date/time on which this record expires
 1503+ cc_expiration varbinary(14) not null
 1504+
 1505+) /*$wgDBTableOptions*/;
 1506+-- composite pk.
 1507+CREATE UNIQUE INDEX /*i*/cc_resource_record ON /*_*/concurrencycheck (cc_resource_type, cc_record);
 1508+-- sometimes there's a delete based on userid.
 1509+CREATE INDEX /*i*/cc_user ON /*_*/concurrencycheck (cc_user);
 1510+-- sometimes there's a delete based on expiration
 1511+CREATE INDEX /*i*/cc_expiration ON /*_*/concurrencycheck (cc_expiration);
 1512+
 1513+
14871514 -- vim: sw=2 sts=2 et
Property changes on: trunk/phase3/maintenance/tables.sql
___________________________________________________________________
Modified: svn:mergeinfo
14881515 Merged /branches/concurrency/maintenance/tables.sql:r108302-108557
Index: trunk/phase3/tests/phpunit/includes/ConcurrencyCheckTest.php
——@@ -0,0 +1,102 @@
 2+<?php
 3+
 4+class ConcurrencyCheckTest extends MediaWikiTestCase {
 5+ /**
 6+ * @var Array of test users
 7+ */
 8+ public static $users;
 9+
 10+ // Prepare test environment
 11+
 12+ public function setUp() {
 13+ parent::setUp();
 14+
 15+ self::$users = array(
 16+ 'user1' => new ApiTestUser(
 17+ 'Concurrencychecktestuser1',
 18+ 'ConcurrencyCheck Test User 1',
 19+ 'concurrency_check_test_user_1@example.com',
 20+ array()
 21+ ),
 22+ 'user2' => new ApiTestUser(
 23+ 'Concurrencychecktestuser2',
 24+ 'ConcurrencyCheck Test User 2',
 25+ 'concurrency_check_test_user_2@example.com',
 26+ array()
 27+ ),
 28+ );
 29+
 30+ // turn on memcached for this test.
 31+ // if no memcached is present, this still works fine.
 32+ global $wgMainCacheType;
 33+ $this->oldcache = $wgMainCacheType;
 34+ $wgMainCacheType = CACHE_MEMCACHED;
 35+ }
 36+
 37+ public function tearDown() {
 38+ // turn off caching again.
 39+ global $wgMainCacheType;
 40+ $wgMainCacheType = $this->oldcache;
 41+
 42+ parent::tearDown();
 43+ }
 44+
 45+ // Actual tests from here down
 46+
 47+ public function testCheckoutCheckin() {
 48+ $first = new ConcurrencyCheck( 'CCUnitTest', self::$users['user1']->user );
 49+ $second = new ConcurrencyCheck( 'CCUnitTest', self::$users['user2']->user );
 50+ $testKey = 1337;
 51+
 52+ // clean up after any previously failed tests
 53+ $first->checkin($testKey);
 54+ $second->checkin($testKey);
 55+
 56+ // tests
 57+ $this->assertTrue( $first->checkout($testKey), "Initial checkout" );
 58+ $this->assertTrue( $first->checkout($testKey), "Cache hit" );
 59+ $this->assertFalse( $second->checkout($testKey), "Checkout of locked resource fails as different user" );
 60+ $this->assertTrue( $first->checkout($testKey), "Checkout of locked resource succeeds as original user" );
 61+ $this->assertFalse( $second->checkin($testKey), "Checkin of locked resource fails as different user" );
 62+ $this->assertTrue( $first->checkin($testKey), "Checkin of locked resource succeeds as original user" );
 63+ $second->setExpirationTime(-5);
 64+ $this->assertTrue( $second->checkout($testKey), "Checked-in resource is now available to second user" );
 65+ $second->setExpirationTime();
 66+ $this->assertTrue( $first->checkout($testKey), "Checkout of expired resource succeeds as first user");
 67+ $this->assertTrue( $second->checkout($testKey, true), "Checkout override" );
 68+ $this->assertFalse( $first->checkout($testKey), "Checkout of overriden resource fails as different user" );
 69+
 70+ // cleanup
 71+ $this->assertTrue( $second->checkin($testKey), "Checkin of record with changed ownership" );
 72+ }
 73+
 74+ public function testExpire() {
 75+ $cc = new ConcurrencyCheck( 'CCUnitTest', self::$users['user1']->user );
 76+ $cc->setExpirationTime(-1);
 77+ $cc->checkout( 1338 ); // these numbers are test record ids.
 78+ $cc->checkout( 1339 );
 79+ $cc->setExpirationTime();
 80+ $cc->checkout( 13310 );
 81+
 82+ // tests
 83+ $this->assertEquals( 2, $cc->expire(), "Resource expiration" );
 84+ $this->assertTrue( $cc->checkin( 13310 ), "Checkin succeeds after expiration" );
 85+ }
 86+
 87+ public function testStatus() {
 88+ $cc = new ConcurrencyCheck( 'CCUnitTest', self::$users['user1']->user );
 89+ $cc->checkout( 1337 );
 90+ $cc->checkout( 1338 );
 91+ $cc->setExpirationTime(-5);
 92+ $cc->checkout( 1339 );
 93+ $cc->setExpirationTime();
 94+
 95+ // tests
 96+ $output = $cc->status( array( 1337, 1338, 1339, 13310 ) );
 97+ $this->assertEquals( true, is_array( $output ), "Status returns values" );
 98+ $this->assertEquals( 4, count( $output ), "Output has the correct number of records" );
 99+ $this->assertEquals( 'valid', $output[1337]['status'], "Current checkouts are listed as valid");
 100+ $this->assertEquals( 'invalid', $output[1339]['status'], "Expired checkouts are invalid");
 101+ $this->assertEquals( 'invalid', $output[13310]['status'], "Missing checkouts are invalid");
 102+ }
 103+}
\ No newline at end of file
Property changes on: trunk/phase3/tests/phpunit/includes/ConcurrencyCheckTest.php
___________________________________________________________________
Added: svn:eol-style
1104 + native
Index: trunk/phase3/tests/phpunit/includes/api/ApiConcurrencyTest.php
——@@ -0,0 +1,171 @@
 2+<?php
 3+
 4+class ApiConcurrencyTest extends ApiTestCase {
 5+ /**
 6+ * @var Array of test users
 7+ */
 8+ public static $users;
 9+
 10+ // Prepare test environment
 11+
 12+ function setUp() {
 13+ parent::setUp();
 14+
 15+ self::$users['one'] = new ApiTestUser(
 16+ 'ApitestuserA',
 17+ 'Api Test UserA',
 18+ 'api_test_userA@example.com',
 19+ array()
 20+ );
 21+
 22+ self::$users['two'] = new ApiTestUser(
 23+ 'ApitestuserB',
 24+ 'Api Test UserB',
 25+ 'api_test_userB@example.com',
 26+ array()
 27+ );
 28+ }
 29+
 30+ public function tearDown() {
 31+ parent::tearDown();
 32+ }
 33+
 34+ function testLogin() {
 35+
 36+ $sessionArray = array();
 37+
 38+ foreach ( self::$users as $key => $user ) {
 39+
 40+ $params = array(
 41+ 'action' => 'login',
 42+ 'lgname' => $user->username,
 43+ 'lgpassword' => $user->password
 44+ );
 45+ list( $result, , $session ) = $this->doApiRequest( $params );
 46+ $this->assertArrayHasKey( "login", $result );
 47+ $this->assertArrayHasKey( "result", $result['login'] );
 48+ $this->assertEquals( "NeedToken", $result['login']['result'] );
 49+ $token = $result['login']['token'];
 50+
 51+ $params = array(
 52+ 'action' => 'login',
 53+ 'lgtoken' => $token,
 54+ 'lgname' => $user->username,
 55+ 'lgpassword' => $user->password
 56+ );
 57+ list( $result, , $session ) = $this->doApiRequest( $params, $session );
 58+ $this->assertArrayHasKey( "login", $result );
 59+ $this->assertArrayHasKey( "result", $result['login'] );
 60+ $this->assertEquals( "Success", $result['login']['result'] );
 61+ $this->assertArrayHasKey( 'lgtoken', $result['login'] );
 62+
 63+ $this->assertNotEmpty( $session, 'API Login must return a session' );
 64+
 65+ $sessionArray[$key] = $session;
 66+
 67+ }
 68+
 69+ return $sessionArray;
 70+
 71+ }
 72+
 73+ /**
 74+ * @depends testLogin
 75+ */
 76+ function testCheckOut( $sessionArray ) {
 77+
 78+ global $wgUser;
 79+
 80+ $wgUser = self::$users['one']->user;
 81+
 82+ list( $result, , $session ) = $this->doApiRequestWithToken( array(
 83+ 'action' => 'concurrency',
 84+ 'ccaction' => 'checkout',
 85+ 'record' => 1,
 86+ 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['one'], self::$users['one']->user );
 87+
 88+ $this->assertEquals( "success", $result['concurrency']['result'] );
 89+
 90+ $wgUser = self::$users['two']->user;
 91+
 92+ list( $result, , $session ) = $this->doApiRequestWithToken( array(
 93+ 'action' => 'concurrency',
 94+ 'ccaction' => 'checkout',
 95+ 'record' => 1,
 96+ 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['two'], self::$users['two']->user );
 97+
 98+ $this->assertEquals( "failure", $result['concurrency']['result'] );
 99+
 100+ list( $result, , $session ) = $this->doApiRequestWithToken( array(
 101+ 'action' => 'concurrency',
 102+ 'ccaction' => 'checkout',
 103+ 'record' => 2,
 104+ 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['two'], self::$users['two']->user );
 105+
 106+ $this->assertEquals( "success", $result['concurrency']['result'] );
 107+
 108+ }
 109+
 110+
 111+ /**
 112+ * @depends testLogin
 113+ */
 114+ function testCheckIn( $sessionArray ) {
 115+
 116+ global $wgUser;
 117+
 118+ $wgUser = self::$users['one']->user;
 119+
 120+ list( $result, , $session ) = $this->doApiRequestWithToken( array(
 121+ 'action' => 'concurrency',
 122+ 'ccaction' => 'checkin',
 123+ 'record' => 1,
 124+ 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['one'], self::$users['one']->user );
 125+
 126+ $this->assertEquals( "success", $result['concurrency']['result'] );
 127+
 128+ list( $result, , $session ) = $this->doApiRequestWithToken( array(
 129+ 'action' => 'concurrency',
 130+ 'ccaction' => 'checkin',
 131+ 'record' => 2,
 132+ 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['one'], self::$users['one']->user );
 133+
 134+ $this->assertEquals( "failure", $result['concurrency']['result'] );
 135+
 136+ $wgUser = self::$users['two']->user;
 137+
 138+ list( $result, , $session ) = $this->doApiRequestWithToken( array(
 139+ 'action' => 'concurrency',
 140+ 'ccaction' => 'checkin',
 141+ 'record' => 2,
 142+ 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['two'], self::$users['two']->user );
 143+
 144+ $this->assertEquals( "success", $result['concurrency']['result'] );
 145+
 146+ }
 147+
 148+ /**
 149+ * @depends testLogin
 150+ */
 151+ function testInvalidCcacton( $sessionArray ) {
 152+ $exception = false;
 153+ try {
 154+ global $wgUser;
 155+
 156+ $wgUser = self::$users['one']->user;
 157+
 158+ list( $result, , $session ) = $this->doApiRequestWithToken( array(
 159+ 'action' => 'concurrency',
 160+ 'ccaction' => 'checkinX',
 161+ 'record' => 1,
 162+ 'resourcetype' => 'responding-to-moodbar-feedback'), $sessionArray['one'], self::$users['one']->user );
 163+ } catch ( UsageException $e ) {
 164+ $exception = true;
 165+ $this->assertEquals("Unrecognized value for parameter 'ccaction': checkinX",
 166+ $e->getMessage() );
 167+ }
 168+ $this->assertTrue( $exception, "Got exception" );
 169+
 170+ }
 171+
 172+}
\ No newline at end of file
Property changes on: trunk/phase3/tests/phpunit/includes/api/ApiConcurrencyTest.php
___________________________________________________________________
Added: svn:eol-style
1173 + native
Index: trunk/phase3/includes/installer/MysqlUpdater.php
——@@ -192,6 +192,7 @@
193193 array( 'modifyField', 'user', 'ug_group', 'patch-ug_group-length-increase.sql' ),
194194 array( 'addField', 'uploadstash', 'us_chunk_inx', 'patch-uploadstash_chunk.sql' ),
195195 array( 'addfield', 'job', 'job_timestamp', 'patch-jobs-add-timestamp.sql' ),
 196+ array( 'addTable', 'concurrencycheck', 'patch-concurrencycheck.sql'),
196197 );
197198 }
198199
Index: trunk/phase3/includes/api/ApiMain.php
——@@ -79,6 +79,7 @@
8080 'patrol' => 'ApiPatrol',
8181 'import' => 'ApiImport',
8282 'userrights' => 'ApiUserrights',
 83+ 'concurrency' => 'ApiConcurrency',
8384 );
8485
8586 /**
Index: trunk/phase3/includes/api/ApiConcurrency.php
——@@ -0,0 +1,107 @@
 2+<?php
 3+
 4+/**
 5+ * API module that handles cooperative locking of web resources
 6+ */
 7+class ApiConcurrency extends ApiBase {
 8+
 9+ public function __construct( $main, $action ) {
 10+ parent::__construct( $main, $action );
 11+ }
 12+
 13+ public function execute() {
 14+ global $wgUser;
 15+
 16+ $this->checkPermission( $wgUser );
 17+
 18+ $params = $this->extractRequestParams();
 19+
 20+ $res = array();
 21+
 22+ $concurrencyCheck = new ConcurrencyCheck( $params['resourcetype'], $wgUser );
 23+
 24+ switch ( $params['ccaction'] ) {
 25+ case 'checkout':
 26+ case 'checkin':
 27+ if ( $concurrencyCheck->$params['ccaction']( $params['record'] ) ) {
 28+ $res['result'] = 'success';
 29+ }
 30+ else {
 31+ $res['result'] = 'failure';
 32+ }
 33+ break;
 34+
 35+ default:
 36+ ApiBase::dieDebug( __METHOD__, "Unhandled concurrency action: {$params['ccaction']}" );
 37+ }
 38+
 39+ $this->getResult()->addValue( null, $this->getModuleName(), $res );
 40+ }
 41+
 42+ public function mustBePosted() {
 43+ return true;
 44+ }
 45+
 46+ public function isWriteMode() {
 47+ return true;
 48+ }
 49+
 50+ public function getAllowedParams() {
 51+ return array(
 52+ 'resourcetype' => array(
 53+ ApiBase::PARAM_TYPE => 'string',
 54+ ApiBase::PARAM_REQUIRED => true
 55+ ),
 56+ 'record' => array(
 57+ ApiBase::PARAM_TYPE => 'integer',
 58+ ApiBase::PARAM_REQUIRED => true
 59+ ),
 60+ 'token' => null,
 61+ 'expiry' => array(
 62+ ApiBase::PARAM_TYPE => 'integer'
 63+ ),
 64+ 'ccaction' => array(
 65+ ApiBase::PARAM_REQUIRED => true,
 66+ ApiBase::PARAM_TYPE => array(
 67+ 'checkout',
 68+ 'checkin',
 69+ ),
 70+ ),
 71+ );
 72+ }
 73+
 74+ public function getParamDescription() {
 75+ return array(
 76+ 'resourcetype' => 'the resource type for concurrency check',
 77+ 'record' => 'an unique identifier for a record of the defined resource type',
 78+ 'expiry' => 'the time interval for expiration',
 79+ 'ccaction' => 'the action for concurrency check',
 80+ );
 81+ }
 82+
 83+ public function getDescription() {
 84+ return 'Get/Set a concurrency check for a web resource type';
 85+ }
 86+
 87+ public function needsToken() {
 88+ return true;
 89+ }
 90+
 91+ public function getTokenSalt() {
 92+ return '';
 93+ }
 94+
 95+ public function getVersion() {
 96+ return __CLASS__ . ': $Id: ApiConcurrency.php $';
 97+ }
 98+
 99+ private function checkPermission( $user ) {
 100+ if ( $user->isAnon() ) {
 101+ $this->dieUsage( "You don't have permission to do that", 'permission-denied' );
 102+ }
 103+ if ( $user->isBlocked( false ) ) {
 104+ $this->dieUsageMsg( array( 'blockedtext' ) );
 105+ }
 106+ }
 107+
 108+}
\ No newline at end of file
Property changes on: trunk/phase3/includes/api/ApiConcurrency.php
___________________________________________________________________
Added: svn:eol-style
1109 + native
Index: trunk/phase3/includes/AutoLoader.php
——@@ -43,6 +43,7 @@
4444 'ChannelFeed' => 'includes/Feed.php',
4545 'Collation' => 'includes/Collation.php',
4646 'ConcatenatedGzipHistoryBlob' => 'includes/HistoryBlob.php',
 47+ 'ConcurrencyCheck' => 'includes/ConcurrencyCheck.php',
4748 'ConfEditor' => 'includes/ConfEditor.php',
4849 'ConfEditorParseError' => 'includes/ConfEditor.php',
4950 'ConfEditorToken' => 'includes/ConfEditor.php',
——@@ -52,7 +53,6 @@
5354 'DeferredUpdates' => 'includes/DeferredUpdates.php',
5455 'DerivativeRequest' => 'includes/WebRequest.php',
5556 'DiffHistoryBlob' => 'includes/HistoryBlob.php',
56 -
5757 'DoubleReplacer' => 'includes/StringUtils.php',
5858 'DummyLinker' => 'includes/Linker.php',
5959 'Dump7ZipOutput' => 'includes/Export.php',
——@@ -271,6 +271,7 @@
272272 'ApiBase' => 'includes/api/ApiBase.php',
273273 'ApiBlock' => 'includes/api/ApiBlock.php',
274274 'ApiComparePages' => 'includes/api/ApiComparePages.php',
 275+ 'ApiConcurrency' => 'includes/api/ApiConcurrency.php',
275276 'ApiDelete' => 'includes/api/ApiDelete.php',
276277 'ApiDisabled' => 'includes/api/ApiDisabled.php',
277278 'ApiEditPage' => 'includes/api/ApiEditPage.php',
Property changes on: trunk/phase3/includes/AutoLoader.php
___________________________________________________________________
Modified: svn:mergeinfo
278279 Merged /branches/concurrency/includes/AutoLoader.php:r108302-108557
Index: trunk/phase3/includes/ConcurrencyCheck.php
——@@ -0,0 +1,330 @@
 2+<?php
 3+
 4+/**
 5+ * Class for cooperative locking of web resources
 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.
 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.
 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.
 18+ *
 19+ * @author Ian Baker <ian@wikimedia.org>
 20+ */
 21+class ConcurrencyCheck {
 22+ /**
 23+ * Constructor
 24+ *
 25+ * @var $resourceType String The calling application or type of resource, conceptually like a namespace
 26+ * @var $user User object, the current user
 27+ * @var $expirationTime Integer (optional) How long should a checkout last, in seconds
 28+ */
 29+ public function __construct( $resourceType, $user, $expirationTime = null ) {
 30+
 31+ // All database calls are to the master, since the whole point of this class is maintaining
 32+ // concurrency. Most reads should come from cache anyway.
 33+ $this->dbw = wfGetDb( DB_MASTER );
 34+
 35+ $this->user = $user;
 36+ // TODO: create a registry of all valid resourceTypes that client app can add to.
 37+ $this->resourceType = $resourceType;
 38+ $this->setExpirationTime( $expirationTime );
 39+ }
 40+
 41+ /**
 42+ * Check out a resource. This establishes an atomically generated, cooperative lock
 43+ * on a key. The lock is tied to the current user.
 44+ *
 45+ * @var $record Integer containing the record id to check out
 46+ * @var $override Boolean (optional) describing whether to override an existing checkout
 47+ * @return boolean
 48+ */
 49+ public function checkout( $record, $override = null ) {
 50+ $memc = wfGetMainCache();
 51+ $this->validateId( $record );
 52+ $dbw = $this->dbw;
 53+ $userId = $this->user->getId();
 54+ $cacheKey = wfMemcKey( $this->resourceType, $record );
 55+
 56+ // when operating with a single memcached cluster, it's reasonable to check the cache here.
 57+ global $wgConcurrencyTrustMemc;
 58+ if( $wgConcurrencyTrustMemc ) {
 59+ $cached = $memc->get( $cacheKey );
 60+ if( $cached ) {
 61+ if( ! $override && $cached['userId'] != $userId && $cached['expiration'] > time() ) {
 62+ // this is already checked out.
 63+ return false;
 64+ }
 65+ }
 66+ }
 67+
 68+ // attempt an insert, check success (this is atomic)
 69+ $insertError = null;
 70+ $res = $dbw->insert(
 71+ 'concurrencycheck',
 72+ array(
 73+ 'cc_resource_type' => $this->resourceType,
 74+ 'cc_record' => $record,
 75+ 'cc_user' => $userId,
 76+ 'cc_expiration' => time() + $this->expirationTime,
 77+ ),
 78+ __METHOD__,
 79+ array('IGNORE')
 80+ );
 81+
 82+ // if the insert succeeded, checkout is done.
 83+ if( $dbw->affectedRows() === 1 ) {
 84+ // delete any existing cache key. can't create a new key here
 85+ // since the insert didn't happen inside a transaction.
 86+ $memc->delete( $cacheKey );
 87+ return true;
 88+ }
 89+
 90+ // if the insert failed, it's necessary to check the expiration.
 91+ $dbw->begin();
 92+ $row = $dbw->selectRow(
 93+ 'concurrencycheck',
 94+ array( 'cc_user', 'cc_expiration' ),
 95+ array(
 96+ 'cc_resource_type' => $this->resourceType,
 97+ 'cc_record' => $record,
 98+ ),
 99+ __METHOD__,
 100+ array()
 101+ );
 102+
 103+ // not checked out by current user, checkout is unexpired, override is unset
 104+ if( ! ( $override || $row->cc_user == $userId || $row->cc_expiration <= time() ) ) {
 105+ // this was a cache miss. populate the cache with data from the db.
 106+ // cache is set to expire at the same time as the checkout, since it'll become invalid then anyway.
 107+ // inside this transaction, a row-level lock is established which ensures cache concurrency
 108+ $memc->set( $cacheKey, array( 'userId' => $row->cc_user, 'expiration' => $row->cc_expiration ), $row->cc_expiration - time() );
 109+ $dbw->rollback();
 110+ return false;
 111+ }
 112+
 113+ $expiration = time() + $this->expirationTime;
 114+
 115+ // execute a replace
 116+ $res = $dbw->replace(
 117+ 'concurrencycheck',
 118+ array( array('cc_resource_type', 'cc_record') ),
 119+ array(
 120+ 'cc_resource_type' => $this->resourceType,
 121+ 'cc_record' => $record,
 122+ 'cc_user' => $userId,
 123+ 'cc_expiration' => $expiration,
 124+ ),
 125+ __METHOD__
 126+ );
 127+
 128+ // cache the result.
 129+ $memc->set( $cacheKey, array( 'userId' => $userId, 'expiration' => $expiration ), $this->expirationTime );
 130+
 131+ $dbw->commit();
 132+ return true;
 133+ }
 134+
 135+ /**
 136+ * Check in a resource. Only works if the resource is checked out by the current user.
 137+ *
 138+ * @var $record Integer containing the record id to checkin
 139+ * @return Boolean
 140+ */
 141+ public function checkin( $record ) {
 142+ $memc = wfGetMainCache();
 143+ $this->validateId( $record );
 144+ $dbw = $this->dbw;
 145+ $userId = $this->user->getId();
 146+ $cacheKey = wfMemcKey( $this->resourceType, $record );
 147+
 148+ $dbw->delete(
 149+ 'concurrencycheck',
 150+ array(
 151+ 'cc_resource_type' => $this->resourceType,
 152+ 'cc_record' => $record,
 153+ 'cc_user' => $userId, // only the owner can perform a checkin
 154+ ),
 155+ __METHOD__,
 156+ array()
 157+ );
 158+
 159+ // check row count (this is atomic, select would not be)
 160+ if( $dbw->affectedRows() === 1 ) {
 161+ $memc->delete( $cacheKey );
 162+ return true;
 163+ }
 164+
 165+ return false;
 166+ }
 167+
 168+ /**
 169+ * Remove all expired checkouts.
 170+ *
 171+ * @return Integer describing the number of records expired.
 172+ */
 173+ public function expire() {
 174+ $memc = wfGetMainCache();
 175+ $dbw = $this->dbw;
 176+ $now = time();
 177+
 178+ // get the rows to remove from cache.
 179+ $res = $dbw->select(
 180+ 'concurrencycheck',
 181+ array( '*' ),
 182+ array(
 183+ 'cc_expiration <= ' . $now,
 184+ ),
 185+ __METHOD__,
 186+ array()
 187+ );
 188+
 189+ // build a list of rows to delete.
 190+ $toExpire = array();
 191+ while( $res && $record = $res->fetchRow() ) {
 192+ $toExpire[] = $record['cc_record'];
 193+ }
 194+
 195+ // remove the rows from the db
 196+ $dbw->delete(
 197+ 'concurrencycheck',
 198+ array(
 199+ 'cc_expiration <= ' . $now,
 200+ ),
 201+ __METHOD__,
 202+ array()
 203+ );
 204+
 205+ // delete all those rows from cache
 206+ // outside a transaction because deletes don't require atomicity.
 207+ foreach( $toExpire as $expire ) {
 208+ $memc->delete( wfMemcKey( $this->resourceType, $expire ) );
 209+ }
 210+
 211+ // return the number of rows removed.
 212+ return $dbw->affectedRows();
 213+ }
 214+
 215+ public function status( $keys ) {
 216+ $memc = wfGetMainCache();
 217+ $dbw = $this->dbw;
 218+ $now = time();
 219+
 220+ $checkouts = array();
 221+ $toSelect = array();
 222+
 223+ // validate keys, attempt to retrieve from cache.
 224+ foreach( $keys as $key ) {
 225+ $this->validateId( $key );
 226+
 227+ $cached = $memc->get( wfMemcKey( $this->resourceType, $key ) );
 228+ if( $cached && $cached['expiration'] > $now ) {
 229+ $checkouts[$key] = array(
 230+ 'status' => 'valid',
 231+ 'cc_resource_type' => $this->resourceType,
 232+ 'cc_record' => $key,
 233+ 'cc_user' => $cached['userId'],
 234+ 'cc_expiration' => $cached['expiration'],
 235+ 'cache' => 'cached',
 236+ );
 237+ } else {
 238+ $toSelect[] = $key;
 239+ }
 240+ }
 241+
 242+ // if there were cache misses...
 243+ if( $toSelect ) {
 244+ // If it's time to go to the database, go ahead and expire old rows.
 245+ $this->expire();
 246+
 247+ // the transaction seems incongruous, I know, but it's to keep the cache update atomic.
 248+ $dbw->begin();
 249+ $res = $dbw->select(
 250+ 'concurrencycheck',
 251+ array( '*' ),
 252+ array(
 253+ 'cc_resource_type' => $this->resourceType,
 254+ 'cc_record IN (' . implode( ',', $toSelect ) . ')',
 255+ 'cc_expiration > unix_timestamp(now())'
 256+ ),
 257+ __METHOD__,
 258+ array()
 259+ );
 260+
 261+ while( $res && $record = $res->fetchRow() ) {
 262+ $record['status'] = 'valid';
 263+ $checkouts[ $record['cc_record'] ] = $record;
 264+
 265+ // safe to store values since this is inside the transaction
 266+ $memc->set(
 267+ wfMemcKey( $this->resourceType, $record['cc_record'] ),
 268+ array( 'userId' => $record['cc_user'], 'expiration' => $record['cc_expiration'] ),
 269+ $record['cc_expiration'] - time()
 270+ );
 271+ }
 272+
 273+ // end the transaction.
 274+ $dbw->rollback();
 275+ }
 276+
 277+ // if a key was passed in but has no (unexpired) checkout, include it in the
 278+ // result set to make things easier and more consistent on the client-side.
 279+ foreach( $keys as $key ) {
 280+ if( ! array_key_exists( $key, $checkouts ) ) {
 281+ $checkouts[$key]['status'] = 'invalid';
 282+ }
 283+ }
 284+
 285+ return $checkouts;
 286+ }
 287+
 288+ public function listCheckouts() {
 289+ // TODO: fill in the function that lets you get the complete set of checkouts for a given application.
 290+ }
 291+
 292+ public function setUser ( $user ) {
 293+ $this->user = $user;
 294+ }
 295+
 296+ public function setExpirationTime ( $expirationTime = null ) {
 297+ global $wgConcurrencyExpirationDefault, $wgConcurrencyExpirationMax, $wgConcurrencyExpirationMin;
 298+
 299+ // check to make sure the time is digits only, so it can be used in queries
 300+ // negative number are allowed, though mostly only used for testing
 301+ if( $expirationTime && preg_match('/^[\d-]+$/', $expirationTime) ) {
 302+ if( $expirationTime > $wgConcurrencyExpirationMax ) {
 303+ $this->expirationTime = $wgConcurrencyExpirationMax; // if the number is too high, limit it to the max value.
 304+ } elseif ( $expirationTime < $wgConcurrencyExpirationMin ) {
 305+ $this->expirationTime = $wgConcurrencyExpirationMin; // low limit, default -1 min
 306+ } else {
 307+ $this->expirationTime = $expirationTime; // the amount of time before a checkout expires.
 308+ }
 309+ } else {
 310+ $this->expirationTime = $wgConcurrencyExpirationDefault; // global default is 15 mins.
 311+ }
 312+ }
 313+
 314+ /**
 315+ * Check to make sure a record ID is numeric, throw an exception if not.
 316+ *
 317+ * @var $record Integer
 318+ * @throws ConcurrencyCheckBadRecordIdException
 319+ * @return boolean
 320+ */
 321+ private static function validateId ( $record ) {
 322+ if( ! preg_match('/^\d+$/', $record) ) {
 323+ throw new ConcurrencyCheckBadRecordIdException( 'Record ID ' . $record . ' must be a positive integer' );
 324+ }
 325+
 326+ // TODO: add a hook here for client-side validation.
 327+ return true;
 328+ }
 329+}
 330+
 331+class ConcurrencyCheckBadRecordIdException extends MWException {};
Property changes on: trunk/phase3/includes/ConcurrencyCheck.php
___________________________________________________________________
Added: svn:eol-style
1332 + native
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -5723,6 +5723,15 @@
57245724 $wgDBtestpassword = '';
57255725
57265726 /**
 5727+ * ConcurrencyCheck keeps track of which web resources are in use, for producing higher-quality UI
 5728+ */
 5729+$wgConcurrencyExpirationDefault = 60 * 15; // Default checkout duration. 15 minutes.
 5730+$wgConcurrencyExpirationMax = 60 * 30; // Maximum possible checkout duration. 30 minutes.
 5731+$wgConcurrencyExpirationMin = 60 * -1; // Minimum possible checkout duration. Negative is possible (but barely) for testing.
 5732+$wgConcurrencyTrustMemc = true; // If running in an environment with multiple discrete caches, set to false.
 5733+
 5734+
 5735+/**
57275736 * For really cool vim folding this needs to be at the end:
57285737 * vim: foldmarker=@{,@} foldmethod=marker
57295738 * @}

Follow-up revisions

RevisionCommit summaryAuthorDate
r108564Updated to use correct cross-db timestamps and date functions...raindrift23:42, 10 January 2012
r108584Fixed concurrency issues related to mysql default locking mode, per Roan's co...raindrift01:10, 11 January 2012
r108585sqlite doesn't actually support read locks. added a workaround until i can d...raindrift01:27, 11 January 2012
r108591sqlite needs its timestamps quoted...raindrift02:02, 11 January 2012
r108595Commenting these tests out so that CI can run, since I need to leave and nobo...raindrift02:48, 11 January 2012
r108601reverts Concurrency works...hashar09:05, 11 January 2012
r108765moving concurrencycheck code here, as of r108591...raindrift21:38, 12 January 2012

Comments

#Comment by Reedy (talk | contribs)   23:12, 10 January 2012

http://integration.mediawiki.org/ci/job/MediaWiki-phpunit/5464/console

Need to do the updater stuff for sqlite too (usually just a svn copy of the DB patch, added to its updater) otherwise you upset jenkins

#Comment by Catrope (talk | contribs)   23:13, 10 January 2012

Breaks unit tests: 

     [exec] There was 1 error:
     [exec] 
     [exec] 1) ConcurrencyCheckTest::testStatus
     [exec] DBQueryError: A database error has occurred.  Did you forget to run maintenance/update.php after upgrading?  See: [https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script]
     [exec] Query: SELECT  *  FROM unittest_concurrencycheck  WHERE cc_resource_type = 'CCUnitTest' AND (cc_record IN (1337,1338,1339,13310)) AND (cc_expiration > unix_timestamp(now()))  
     [exec] Function: ConcurrencyCheck::status
     [exec] Error: 1 no such function: now
     [exec] #0 /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/db/Database.php(888): DatabaseBase->reportQueryError('no such functio...', 1, 'SELECT  *  FROM...', 'ConcurrencyChec...', false)
     [exec] #1 /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/db/Database.php(1361): DatabaseBase->query('SELECT  *  FROM...', 'ConcurrencyChec...')
     [exec] #2 /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/ConcurrencyCheck.php(258): DatabaseBase->select('concurrencychec...', Array, Array, 'ConcurrencyChec...', Array)
     [exec] #3 /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/ConcurrencyCheckTest.php(95): ConcurrencyCheck->status(Array)
     [exec] #4 [internal function]: ConcurrencyCheckTest->testStatus()
     [exec] #5 /usr/share/php/PHPUnit/Framework/TestCase.php(738): ReflectionMethod->invokeArgs(Object(ConcurrencyCheckTest), Array)
     [exec] #6 /usr/share/php/PHPUnit/Framework/TestCase.php(628): PHPUnit_Framework_TestCase->runTest()
     [exec] #7 /usr/share/php/PHPUnit/Framework/TestResult.php(666): PHPUnit_Framework_TestCase->runBare()
     [exec] #8 /usr/share/php/PHPUnit/Framework/TestCase.php(576): PHPUnit_Framework_TestResult->run(Object(ConcurrencyCheckTest))
     [exec] #9 /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php(70): PHPUnit_Framework_TestCase->run(Object(PHPUnit_Framework_TestResult))
     [exec] #10 /usr/share/php/PHPUnit/Framework/TestSuite.php(757): MediaWikiTestCase->run(Object(PHPUnit_Framework_TestResult))
     [exec] #11 /usr/share/php/PHPUnit/Framework/TestSuite.php(733): PHPUnit_Framework_TestSuite->runTest(Object(ConcurrencyCheckTest), Object(PHPUnit_Framework_TestResult))
     [exec] #12 /usr/share/php/PHPUnit/Framework/TestSuite.php(693): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult), false, Array, Array, false)
     [exec] #13 /usr/share/php/PHPUnit/Framework/TestSuite.php(693): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult), false, Array, Array, false)
     [exec] #14 /usr/share/php/PHPUnit/TextUI/TestRunner.php(305): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult), false, Array, Array, false)
     [exec] #15 /usr/share/php/PHPUnit/TextUI/Command.php(188): PHPUnit_TextUI_TestRunner->doRun(Object(PHPUnit_Framework_TestSuite), Array)
     [exec] #16 /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php(44): PHPUnit_TextUI_Command->run(Array, true)
     [exec] #17 /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php(60): MediaWikiPHPUnitCommand::main()
     [exec] #18 {main}
     [exec] 
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/db/Database.php:921
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/db/Database.php:888
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/db/Database.php:1361
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/ConcurrencyCheck.php:258
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/ConcurrencyCheckTest.php:95
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php:70
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php:44
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php:60
     [exec] 
     [exec] --
     [exec] 
     [exec] 
     [exec] There were 5 failures:
     [exec] 
     [exec] 1) DatabaseSqliteTest::testUpgrades
     [exec] Different tables upgrading from 1.15 to 1.19alpha
     [exec] Failed asserting that two arrays are equal.
     [exec] --- Expected
     [exec] +++ Actual
     [exec] @@ @@
     [exec]      [3] => change_tag
     [exec] -    [4] => concurrencycheck
     [exec] -    [5] => config
     [exec] -    [6] => external_user
     [exec] -    [7] => externallinks
     [exec] -    [8] => filearchive
     [exec] -    [9] => hitcounter
     [exec] -    [10] => image
     [exec] -    [11] => imagelinks
     [exec] -    [12] => interwiki
     [exec] -    [13] => ipblocks
     [exec] -    [14] => iwlinks
     [exec] -    [15] => job
     [exec] -    [16] => l10n_cache
     [exec] -    [17] => langlinks
     [exec] -    [18] => log_search
     [exec] -    [19] => logging
     [exec] -    [20] => module_deps
     [exec] -    [21] => msg_resource
     [exec] -    [22] => msg_resource_links
     [exec] -    [23] => objectcache
     [exec] -    [24] => oldimage
     [exec] -    [25] => page
     [exec] -    [26] => page_props
     [exec] -    [27] => page_restrictions
     [exec] -    [28] => pagelinks
     [exec] -    [29] => protected_titles
     [exec] -    [30] => querycache
     [exec] -    [31] => querycache_info
     [exec] -    [32] => querycachetwo
     [exec] -    [33] => recentchanges
     [exec] -    [34] => redirect
     [exec] -    [35] => revision
     [exec] -    [36] => site_stats
     [exec] -    [37] => tag_summary
     [exec] -    [38] => templatelinks
     [exec] -    [39] => text
     [exec] -    [40] => transcache
     [exec] -    [41] => updatelog
     [exec] -    [42] => uploadstash
     [exec] -    [43] => user
     [exec] -    [44] => user_former_groups
     [exec] -    [45] => user_groups
     [exec] -    [46] => user_newtalk
     [exec] -    [47] => user_properties
     [exec] -    [48] => valid_tag
     [exec] -    [49] => watchlist
     [exec] +    [4] => config
     [exec] +    [5] => external_user
     [exec] +    [6] => externallinks
     [exec] +    [7] => filearchive
     [exec] +    [8] => hitcounter
     [exec] +    [9] => image
     [exec] +    [10] => imagelinks
     [exec] +    [11] => interwiki
     [exec] +    [12] => ipblocks
     [exec] +    [13] => iwlinks
     [exec] +    [14] => job
     [exec] +    [15] => l10n_cache
     [exec] +    [16] => langlinks
     [exec] +    [17] => log_search
     [exec] +    [18] => logging
     [exec] +    [19] => module_deps
     [exec] +    [20] => msg_resource
     [exec] +    [21] => msg_resource_links
     [exec] +    [22] => objectcache
     [exec] +    [23] => oldimage
     [exec] +    [24] => page
     [exec] +    [25] => page_props
     [exec] +    [26] => page_restrictions
     [exec] +    [27] => pagelinks
     [exec] +    [28] => protected_titles
     [exec] +    [29] => querycache
     [exec] +    [30] => querycache_info
     [exec] +    [31] => querycachetwo
     [exec] +    [32] => recentchanges
     [exec] +    [33] => redirect
     [exec] +    [34] => revision
     [exec] +    [35] => site_stats
     [exec] +    [36] => tag_summary
     [exec] +    [37] => templatelinks
     [exec] +    [38] => text
     [exec] +    [39] => transcache
     [exec] +    [40] => updatelog
     [exec] +    [41] => uploadstash
     [exec] +    [42] => user
     [exec] +    [43] => user_former_groups
     [exec] +    [44] => user_groups
     [exec] +    [45] => user_newtalk
     [exec] +    [46] => user_properties
     [exec] +    [47] => valid_tag
     [exec] +    [48] => watchlist
     [exec]  )
     [exec] 
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/db/DatabaseSqliteTest.php:211
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php:70
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php:44
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php:60
     [exec] 
     [exec] 2) ConcurrencyCheckTest::testCheckoutCheckin
     [exec] Checkout of locked resource fails as different user
     [exec] Failed asserting that <boolean:true> is false.
     [exec] 
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/ConcurrencyCheckTest.php:58
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php:70
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php:44
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php:60
     [exec] 
     [exec] 3) ConcurrencyCheckTest::testExpire
     [exec] Resource expiration
     [exec] Failed asserting that <integer:0> matches expected <integer:2>.
     [exec] 
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/ConcurrencyCheckTest.php:82
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php:70
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php:44
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php:60
     [exec] 
     [exec] 4) ApiConcurrencyTest::testCheckOut
     [exec] Failed asserting that two strings are equal.
     [exec] --- Expected
     [exec] +++ Actual
     [exec] @@ @@
     [exec] -failure
     [exec] +success
     [exec] 
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/api/ApiConcurrencyTest.php:97
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php:70
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php:44
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php:60
     [exec] 
     [exec] 5) ApiConcurrencyTest::testCheckIn
     [exec] Failed asserting that two strings are equal.
     [exec] --- Expected
     [exec] +++ Actual
     [exec] @@ @@
     [exec] -success
     [exec] +failure
     [exec] 
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/includes/api/ApiConcurrencyTest.php:143
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiTestCase.php:70
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/MediaWikiPHPUnitCommand.php:44
     [exec] /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/tests/phpunit/phpunit.php:60
     [exec]
#Comment by Catrope (talk | contribs)   23:48, 10 January 2012 
	 $wgConcurrencyExpirationMin = 60 * -1; // Minimum possible checkout duration.  Negative is possible (but barely) for testing.

What does "barely possible" mean? Why is this set to a negative value by default while that's apparently discouraged? 

$memc = wfGetMainCache();

Why not use $wgMemc like most things do? 

$cacheKey = wfMemcKey( $this->resourceType, $record );

Please prefix this with 'concurrencycheck' 

				'cc_expiration' => time() + $this->expirationTime,

You're using UNIX timestamps in the DB instead of MW timestamps? Everything else in MW uses the DB-specific timestamp format (14-character TS_MW timestamps for MySQL and SQLite, and God-knows-what for Postgres), you may want to do that too for consistency (although I see how using UNIX timestamps everywhere makes things easy). To format a timestamp for DB consumption, use $db->timestamp( $ts ) , to convert it back to a UNIX timestamp, use wfTimestamp( TS_UNIX, $ts ) .

The logic in ConcurrencyCheck::checkout() is flawed and allows for race conditions. One thing that can happen is that two processes try to INSERT a lock, but both fail because there is a previous, expired lock. They will then both execute the REPLACE query (which will insert identical data), and both will think they have the lock. Instead, you should first issue a DELETE query to delete any expired or same-user locks (or any locks if $override is set; that parameter looks dangerous and should be documented as such) that might exist, then do the INSERT stuff. That way everything is atomic. Generally, if you're doing a SELECT, followed by an if statement, followed by a write query, you're [probably doing something wrong. The fact that things happen in a transaction may or may not mitigate this, I'm not sure, but doing it without transactions or locking is always better.

In checkin(), you're checking whether the DELETE did anything before removing the memc key. I don't understand why you're doing this, and you should probably delete the memc key unconditionally here.

expire() is weird because you're selecting first, then deleting, only so you can delete from memc. But the memc entries already have expiry timestamps set, so this isn't necessary. 

					'cc_record IN (' . implode( ',', $toSelect ) . ')',

Don't do this, use 'cc_record' => $toSelect, .

In status(), you don't need to check for freshness right after calling expire(), do you? 

			// the transaction seems incongruous, I know, but it's to keep the cache update atomic.

That makes no sense to me whatsoever. Memc updates don't behave differently inside or outside of database transactions, unless you happen to be using CACHE_DB. 

		// check to make sure the time is digits only, so it can be used in queries
		if( $expirationTime && preg_match('/^[\d-]+$/', $expirationTime) ) {

Just cast it to an integer. 

		if( ! preg_match('/^\d+$/', $record) ) {

Again, it's simpler to just cast to an integer, check for >0 (the current code lets 0 slip through), then cast back to a string and compare with the input.

The rest of the revision is OK.

#Comment by Siebrand (talk | contribs)   23:54, 10 January 2012

Isn't this exactly the kind of stuff we didn't want to be put in trunk during the slush, or did I miss something?

#Comment by Catrope (talk | contribs)   23:55, 10 January 2012

I guess you could argue that. However, this adds dead code that's not actually used by anything (except for MoodBar).

I'll ask Chad or Brion for a verdict.

#Comment by Brion VIBBER (talk | contribs)   00:03, 11 January 2012

Schema update (clean table), new api methods, is being coded for a particular extension but is a generalizable thing, there are some worries about the logic of it.

I'd recommend making this part of the one extension that immediately needs it for now. If it's proven to work reliably, we could consider merging to trunk in future.

Even a simple schema update means you need to make sure it works for all database types -- adding new tables right before release stabilization is frowned upon. :)

#Comment by Catrope (talk | contribs)   00:09, 11 January 2012

Alright. Let's give Ian some time to stabilize it, then move it into the MoodBar extension tomorrow. Should be simple enough.

Status & tagging log