MediaWiki r83140 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r83139‎ | r83140 (on ViewVC)‎ | r83141 >
Date:09:37, 3 March 2011
Author:tstarling
Status:resolved (Comments)
Tags:
Comment:
* Rewrote ObjectCache.php to conform to the modern coding style, and to be less convoluted about how CACHE_ANYTHING and CACHE_ACCEL are resolved. Moved most functionality to static members of a new ObjectCache class.
* Moved the global functions to GlobalFunctions.php, where they are now just convenience wrappers. Made them return non-references. Updated callers (none found in extensions).
* Added an advanced configuration method, $wgObjectCaches, which allows a lot more detail in the object cache configuration than $wgMainCacheType.
* Made all object cache classes derive from BagOStuff.
* Split the MWMemcached class into a generic client class and a MediaWiki-specific wrapper class. The wrapper class presents a simple BagOStuff interface to calling code, hiding memcached client internals, and will simplify the task of supporting the PECL extension.
* Added some extra constructor parameters to MWMemcached, configurable via $wgObjectCaches.
* Removed the *_multi() methods from BagOStuff, my grepping indicates that they are not used.
* Rewrote FakeMemCachedClient as a BagOStuff subclass, called EmptyBagOStuff.
* Added an optional "server" parameter to SQLBagOStuff. This allows the server holding the objectcache table to be different from the server holding the core DB.
* Added MultiWriteBagOStuff: a cache class which writes to multiple locations, and reads from them in a defined fallback sequence. This can be used to extend the cache space by adding disk-backed storage to existing in-memory caches.
* Made MWMemcached::get() return false on failure instead of null, to match the BagOStuff documentation and the other BagOStuff subclasses. Anything that was relying on it returning null would have already been broken with SqlBagOStuff.
* Fixed a bug in the memcached client causing keys with spaces or line breaks in them to break the memcached protocol, injecting arbitrary commands or parameters. Since the PECL client apparently also has this flaw, I implemented the fix in the wrapper class.
* Renamed BagOStuff::set_debug() to setDebug(), since we aren't emulating the memcached client anymore
* Fixed spelling error in MWMemcached: persistant -> persistent
Modified paths:

Diff [purge]

Loading diff…

Sign-offs

UserFlagDate
^demoninspected15:36, 3 March 2011

Follow-up revisions

Rev.Commit summaryAuthorDate
r83144Followup r83140: FakeMemCachedClient -> EmptyBagOStuff in testsdemon12:55, 3 March 2011
r83147* When CACHE_ANYTHING is requested, return the cached instance, don't constru...tstarling15:24, 3 March 2011
r83208* Added an Ehcache client....tstarling06:01, 4 March 2011
r83416Follow up r83140. Add a clear() method to ObjectCache so that it can be used ...platonides23:07, 6 March 2011
r83418r82867 converted $wgCaches into a class instance. Update the parsertests....platonides23:15, 6 March 2011
r83419$wgCaches removed in r83140. Installer.php no longer uses $errs and $mainList...platonides23:22, 6 March 2011
r83878Fix r83140: 'ObjectCache::newAnything' is not a valid callback, use array( 'O...catrope10:27, 14 March 2011
r84727Merged in the ObjectCache refactor and the Ehcache client. MFT r83135, r83136...tstarling03:28, 25 March 2011
r89106Followup r83140, fix undefined $idreedy14:28, 29 May 2011
r105419(bug 32853) DBA cache broken in MW 1.18...hashar11:04, 7 December 2011

Past revisions this follows-up on

Rev.Commit summaryAuthorDate
r83136More renames and splits for objectcache reorganisation.tstarling04:48, 3 March 2011

Comments

#Comment by Nikerabbit (talk | contribs)   09:56, 3 March 2011
  • Made MWMemcached::get() return false on failure instead of null, to match the BagOStuff documentation and the other BagOStuff subclasses. Anything that was relying on it returning null would have already been broken with SqlBagOStuff.

This is good! It has caused so many subtle bugs.

#Comment by IAlex (talk | contribs)   10:07, 3 March 2011
+	CACHE_NONE => array( 'class' => 'FakeMemCachedClient' ),

Shouldn't the class be EmptyBagOStuff ?

#Comment by Tim Starling (talk | contribs)   10:54, 3 March 2011

It should. There are quite a lot of references to FakeMemCachedClient in the core. I wanted to fix them in a separate commit to make review easier.

#Comment by ^demon (talk | contribs)   12:57, 3 March 2011

Fixed the instances in tests/ in r83144.

#Comment by Siebrand (talk | contribs)   15:25, 3 March 2011

Causes PHP Fatal error: Call to a member function set() on a non-object in /www/w/includes/MemcachedSessions.php on line 67. Tim can kind of reproduce this. His latest comments from IRC: "My test wiki just started reproducing it. Actually I think it is PHP's fault after all. I'm reproducing it on 5.3.4. What I don't understand is how it could ever work."

Marking FIXME.

#Comment by Tim Starling (talk | contribs)   15:25, 3 March 2011

Should be fixed as of r83147.

#Comment by Tim Starling (talk | contribs)   15:52, 3 March 2011

On request shutdown, PHP does things in the following order:

  1. register_shutdown_function() functions
  2. zend_call_destructors()
  3. zend_deactivate_modules()

zend_call_destructors() goes through the $GLOBALS symbol table and identifies every global variable that holds an object which has a reference count of 1. It removes these globals. It doesn't decrement any reference counts. Any globals which are not objects, or have a reference count greater than 1, are left in the symbol table. So if we have a variable $x:

$x = new Foo;

That will be deleted, unless we protect it by making a reference to it from somewhere:

$y =& $x;

After this perplexing action, zend_deactivate_modules() calls php_session_save_current_state() which calls our memcached save handler, which uses $wgMemc. Because this revision removed all references to $wgMemc, being obsolete holdovers from PHP 4, this exposed $wgMemc to zend_call_destructors(), meaning that it was not present in the symbol table when memsess_write() was called.

#Comment by Siebrand (talk | contribs)   16:01, 3 March 2011

Thanks for digging, Tim. Back to new.

#Comment by Tim Starling (talk | contribs)   00:04, 4 March 2011
#Comment by Siebrand (talk | contribs)   23:04, 5 March 2011

Marked WONTFIX upstream. Made a choice between two evils, apparently.

#Comment by Tim Starling (talk | contribs)   00:19, 6 March 2011

I talked to Johannes on IRC about it. I think I convinced him that something is not quite right about this code. He said he'd have another look at it if I wrote it up for php internals.

Filing bugs against PHP is more of a starting point than an ending point. Most of the bugs that I've gotten fixed in PHP started out being closed as some flavour of "bogus".

#Comment by Reedy (talk | contribs)   10:51, 25 March 2011

newFromParams() in ObjectCache.php:

Line 55 is using an undefined variable $id

Status & tagging log

  • 00:28, 15 June 2011 ^demon (talk | contribs) changed the status of r83140 [removed: new added: resolved]
  • 14:28, 29 May 2011 Reedy (talk | contribs) changed the status of r83140 [removed: fixme added: new]
  • 10:51, 25 March 2011 Reedy (talk | contribs) changed the status of r83140 [removed: new added: fixme]
  • 16:01, 3 March 2011 Siebrand (talk | contribs) changed the status of r83140 [removed: fixme added: new]
  • 15:25, 3 March 2011 Siebrand (talk | contribs) changed the status of r83140 [removed: new added: fixme]