Fix transaction nesting caused by LocalFile.

The transaction bracket in LocalFile::recordUpload2 used to span a call
to WikiPage::doEdit, which in turn opens a transaction. Nesting transactions
this way does not work: the first transaction is committed prematurely when
the second one starts. This may cause serious database corruption and
generally exciting behavior.

This change commits LocalFile's own transaction before any interaction
with WikiPage. There may be a race condition here, but that case seems to
be already handled in the code. Also, in the previous "broken" state, all
transactional protection got lost anyway - so this should at least be no
worse than what we had.

This may be changed back if and when we have support for nested
transactions.

Change-Id: I20d90fedb2d19c64ccf0f3942ddda756fe511c12

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index 2630193..695c4e9 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -1154,7 +1154,9 @@ class LocalFile extends File {
                $logId = $log->addEntry( $action, $descTitle, $comment, array(), $user );
 
                wfProfileIn( __METHOD__ . '-edit' );
-               if ( $descTitle->exists() ) {
+               $exists = $descTitle->exists();
+
+               if ( $exists ) {
                        # Create a null revision
                        $latest = $descTitle->getLatestRevID();
                        $nullRevision = Revision::newNullRevision(
@@ -1169,6 +1171,15 @@ class LocalFile extends File {
                                wfRunHooks( 'NewRevisionFromEditComplete', array( $wikiPage, $nullRevision, $latest, $user ) );
                                $wikiPage->updateRevisionOn( $dbw, $nullRevision );
                        }
+               }
+
+               # Commit the transaction now, in case something goes wrong later
+               # The most important thing is that files don't get lost, especially archives
+               # NOTE: once we have support for nested transactions, the commit may be moved
+               #       to after $wikiPage->doEdit has been called.
+               $dbw->commit( __METHOD__ );
+
+               if ( $exists ) {
                        # Invalidate the cache for the description page
                        $descTitle->invalidateCache();
                        $descTitle->purgeSquid();
@@ -1178,16 +1189,18 @@ class LocalFile extends File {
                        # Squid and file cache for the description page are purged by doEdit.
                        $status = $wikiPage->doEdit( $pageText, $comment, EDIT_NEW | EDIT_SUPPRESS_RC, false, $user );
 
-                       if ( isset( $status->value['revision'] ) ) {
-                               $dbw->update( 'logging', array( 'log_page' => $status->value['revision']->getPage() ), array( 'log_id' => $logId ), __METHOD__ );
+                       if ( isset( $status->value['revision'] ) ) { // XXX; doEdit() uses a transaction
+                               $dbw->begin();
+                               $dbw->update( 'logging',
+                                       array( 'log_page' => $status->value['revision']->getPage() ),
+                                       array( 'log_id' => $logId ),
+                                       __METHOD__
+                               );
+                               $dbw->commit(); // commit before anything bad can happen
                        }
                }
                wfProfileOut( __METHOD__ . '-edit' );
 
-               # Commit the transaction now, in case something goes wrong later
-               # The most important thing is that files don't get lost, especially archives
-               $dbw->commit( __METHOD__ );
-
                # Save to cache and purge the squid
                # We shall not saveToCache before the commit since otherwise
                # in case of a rollback there is an usable file from memcached