r83868 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r83867‎ | r83868 | r83869 >
Date:04:26, 14 March 2011
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Revert r79122, causes bug 27891 (old version seen immediately after edit).
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Article.php
@@ -40,7 +40,7 @@
4141 var $mTouched = '19700101000000'; // !<
4242 var $mUser = -1; // !< Not loaded
4343 var $mUserText = ''; // !< username from Revision if set
44 - var $mParserOptions; // !< ParserOptions object for $wgUser articles
 44+ var $mParserOptions; // !< ParserOptions object
4545 var $mParserOutput; // !< ParserCache object if set
4646 /**@}}*/
4747
@@ -3610,7 +3610,7 @@
36113611 $edit->revid = $revid;
36123612 $edit->newText = $text;
36133613 $edit->pst = $this->preSaveTransform( $text, $user, $popts );
3614 - $edit->popts = $this->getParserOptions( true );
 3614+ $edit->popts = $this->getParserOptions();
36153615 $edit->output = $wgParser->parse( $edit->pst, $this->mTitle, $edit->popts, true, true, $revid );
36163616 $edit->oldText = $this->getRawText();
36173617
@@ -4395,23 +4395,15 @@
43964396
43974397 /**
43984398 * Get parser options suitable for rendering the primary article wikitext
4399 - * @param $canonical boolean Determines that the generated must not depend on user preferences (see bug 14404)
44004399 * @return mixed ParserOptions object or boolean false
44014400 */
4402 - public function getParserOptions( $canonical = false ) {
4403 - global $wgUser, $wgLanguageCode;
 4401+ public function getParserOptions() {
 4402+ global $wgUser;
44044403
4405 - if ( !$this->mParserOptions || $canonical ) {
4406 - $user = !$canonical ? $wgUser : new User;
4407 - $parserOptions = new ParserOptions( $user );
4408 - $parserOptions->setTidy( true );
4409 - $parserOptions->enableLimitReport();
4410 -
4411 - if ( $canonical ) {
4412 - $parserOptions->setUserLang( $wgLanguageCode ); # Must be set explicitely
4413 - return $parserOptions;
4414 - }
4415 - $this->mParserOptions = $parserOptions;
 4404+ if ( !$this->mParserOptions ) {
 4405+ $this->mParserOptions = new ParserOptions( $wgUser );
 4406+ $this->mParserOptions->setTidy( true );
 4407+ $this->mParserOptions->enableLimitReport();
44164408 }
44174409
44184410 // Clone to allow modifications of the return value without affecting

Follow-up revisions

RevisionCommit summaryAuthorDate
r83869Revert r79130, equivalent to trunk r83868: use the same cache before and afte...tstarling04:29, 14 March 2011
r83870Revert r79130, equivalent to trunk r83868: use the same cache before and afte...tstarling04:31, 14 March 2011
r83994Logging hack for bug 27891. Committing so I can revert it without being too sad.tstarling03:04, 15 March 2011
r85618Various fixes for PHPUnit tests:...pcopp14:54, 7 April 2011
r87234Re-adding the logging hack from r83994 since bug 27891 has been reopenedtstarling01:57, 2 May 2011
r87235Remove the session_write_close() from r72475 since it apparently causes bug 2...tstarling04:21, 2 May 2011
r88904(bug 27891) Follow-up r72475: destroy the LBFactory singleton before doing an...btongminh17:11, 26 May 2011
r89706Reinstate r79122 (fix for bug 14404), reverting r83868. The real bug seem to ...platonides22:28, 7 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r79122Fix bug 14404. The articles are now always saved with the default options....platonides18:44, 28 December 2010

Comments

#Comment by Platonides (talk | contribs)   17:40, 15 March 2011

The article should have been reparsed on page view. r79122 should have only made that page view a bit slower.

We could perform two parses there, one with the default options and a second one with the user one if different (that was what I expected it would be needed from reading bug 14404), but as it seems to just expose a bug located somewhere else, doesn't seem appropiate.

How are memcached timeouts leading to viewing the old version?

#Comment by Tim Starling (talk | contribs)   22:28, 15 March 2011

When you save an article, the current master position is saved into the session. When you are redirected and view the page a couple of seconds later, the master position is fetched out of the session. A random slave server is selected with lag less than 30s. When the first query is done on the slave, it waits for up to 10s for the slave to catch up with the saved master position. If this wait times out, then reads continue anyway, and stale data is returned.

Due to such a timeout, it's possible for Article to get the wrong page_touched and page_latest. If there is an old parser cache entry present, then the stale page_touched will cause it to be returned.

If there is no parser cache entry present, then the old version of the article will be retrieved, parsed and saved to the parser cache with a timestamp greater than the real page_touched in the master. A parser cache entry saved in this way will persist until it is purged with an edit or action=purge. It's possible that this condition is also triggered by other users viewing the article at around the time of the save.

Reverting the bug fix was a simple and safe way to regain the less-broken 1.16 behaviour. A better bug fix, perhaps addressing bug 27891 in a different way, can now be developed, tested and reviewed.

Memcached timeouts are not related.

Status & tagging log