r98020 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98019‎ | r98020 | r98021 >
Date:20:07, 24 September 2011
Author:mglaser
Status:reverted (Comments)
Tags:
Comment:
* restore DB connection after unserialization. This is needed in all DB variants safe for MySQL. Found when running PagedTiffHandler Selenium tests on Postgres. Credits to Dan Nessett.
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/Database.php
——@@ -608,6 +608,17 @@
609609 }
610610 }
611611
 612+ /**
 613+ * Called by unserialize. Needed to reopen DB connection, which
 614+ * is not saved by serialize.
 615+ */
 616+ public function __wakeup() {
 617+ if ( $this->isOpen() ) {
 618+ $this->open( $this->mServer, $this->mUser,
 619+ $this->mPassword, $this->mDBname);
 620+ }
 621+ }
 622+
612623 /**
613624 * Same as new DatabaseMysql( ... ), kept for backward compatibility
614625 * @deprecated since 1.17

Follow-up revisions

RevisionCommit summaryAuthorDate
r99835follow-up to r98020. Throw MWException on attempt to serialize database.mglaser01:33, 15 October 2011

Comments

#Comment by MaxSem (talk | contribs)   20:29, 24 September 2011

I tried wandering in this direction some time ago with SQLite and came to conclusion that although this approach can help to hide one particular bug, overall it causes more problems than it fixes. For example:

  • By quietly reconnecting to database you don't restore connection properties set outside of open(). Oops, $wgSharedDB quietly disconnected on SQLite.
  • Extra connection to the same server can cause problems on MySQL and SQLite.
  • I'm pretty sure that LoadBalancer doesn't like it either.

Summary: instead of resorting to this hack, I recommend you to fix the code path that leads to serialization/unserialization of DB connection in the first place and add function __sleep() { throw new MWException; } to DatabaseBase instead.

#Comment by Mglaser (talk | contribs)   01:36, 15 October 2011

Fixed in r99835. As MaxSem suggested, I put in a MWException on attempt to serialize the Database object. In my tests, no exception is thrown. I assume the serialization issue that lead to r98020 was fixed before.

#Comment by MaxSem (talk | contribs)   02:03, 15 October 2011

Yeah, the culprit appears to be fixed by me in r75228 aeons ago, but bug report that caused this revision appears to be even older.

Status & tagging log