"Catrope" changed the status of MediaWiki.r108559 to "fixme" and commented it.
URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/108559#c29325
Old Status: new
> New Status: fixme
Commit summary for MediaWiki.r108559:
MERGE branches/concurrency 108301:108557 into trunk
Catrope's comment:
<pre>
$wgConcurrencyExpirationMin = 60 * -1; // Minimum possible checkout
duration. Negative is possible (but barely) for testing.
</pre>
What does "barely possible" mean? Why is this set to a negative value by
default while that's apparently discouraged?
<pre>
$memc = wfGetMainCache();
</pre>
Why not use $wgMemc like most things do?
<pre>
$cacheKey = wfMemcKey( $this->resourceType, $record );
</pre>
Please prefix this with 'concurrencycheck'
<pre>
'cc_expiration' => time() +
$this->expirationTime,
</pre>
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
<code>$db->timestamp( $ts )</code> , to convert it back to a UNIX timestamp,
use <code>wfTimestamp( TS_UNIX, $ts )</code> .
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.
<pre>
'cc_record IN (' . implode( ',',
$toSelect ) . ')',
</pre>
Don't do this, use <code>'cc_record' => $toSelect,</code> .
In status(), you don't need to check for freshness right after calling
expire(), do you?
<pre>
// the transaction seems incongruous, I know, but it's
to keep the cache update atomic.
</pre>
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.
<pre>
// check to make sure the time is digits only, so it can be
used in queries
if( $expirationTime && preg_match('/^[\d-]+$/',
$expirationTime) ) {
</pre>
Just cast it to an integer.
<pre>
if( ! preg_match('/^\d+$/', $record) ) {
</pre>
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.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
