r87235 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r87234‎ | r87235 | r87236 >
Date:04:21, 2 May 2011
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Remove the session_write_close() from r72475 since it apparently causes bug 27891, total breakage of ChronologyProtector. WMF patch only for now, maybe Bryan will fix it properly.
Modified paths:
  • /branches/wmf/1.17wmf1/includes/Wiki.php (modified) (history)

Diff [purge]

Index: branches/wmf/1.17wmf1/includes/Wiki.php
@@ -374,7 +374,12 @@
375375 // Do any deferred jobs
376376 wfDoUpdates( true );
377377 // Close the session so that jobs don't access the current session
378 - session_write_close();
 378+ /**
 379+ * WMF PATCH: removed, causes bug 27891.
 380+ * Removing this here without taking other steps probably breaks
 381+ * $wgAllowCopyUploads.
 382+ * session_write_close();
 383+ */
379384 $this->doJobs();
380385 wfProfileOut( __METHOD__ );
381386 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r88902Merge r87235 to 1.17, fix will have to go in 1.18.btongminh16:57, 26 May 2011
r88904(bug 27891) Follow-up r72475: destroy the LBFactory singleton before doing an...btongminh17:11, 26 May 2011
r90293Per comments revert r88904 and remove session_write_close entirely. This will...btongminh16:42, 17 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r72475Follow-up r70137: Made asynchronous upload working a bit more. It now fully w...btongminh10:18, 6 September 2010
r83868Revert r79122, causes bug 27891 (old version seen immediately after edit).tstarling04:26, 14 March 2011
r83994Logging hack for bug 27891. Committing so I can revert it without being too sad.tstarling03:04, 15 March 2011
r87234Re-adding the logging hack from r83994 since bug 27891 has been reopenedtstarling01:57, 2 May 2011

Comments

#Comment by Bryan (talk | contribs)   07:32, 2 May 2011

I'll look into it later this week.

#Comment by Bryan (talk | contribs)   17:54, 5 May 2011

If I understand correctly, a loadbalancer factory should be considered session specific. The solution would then be to shutdown the current factory before executing jobs and creating a new factory for the jobs.

#Comment by Tim Starling (talk | contribs)   23:25, 5 May 2011

Could do, I guess. Why is it necessary to prevent jobs from accessing the session? What bug does that fix?

#Comment by Bryan (talk | contribs)   20:28, 6 May 2011

When using asynchronous upload by url, UploadFromUrlJob stores the result of the upload in the session, so that API clients can retrieve the status. This session is obviously not the same session as the request session, but that of the user who initiated the upload. It therefore calls wfSetupSession() with the session id of the uploader.

This would all be unnecessary of course if jobs had a way to communicate their results back, but currently there is none.

#Comment by Tim Starling (talk | contribs)   00:52, 2 June 2011

This seems like a good reason to not use the PHP session extension. I've been talking for a while about getting rid of it, it doesn't really provide much value for us. If we had an object-oriented view of a session, then you could create an object which accessed the uploader's session, without changing the definition of $_SESSION.

I'm not really keen about closing all database connections and then reopening them just to get some session-related side-effect.

A possible interim solution would be to implement an object-oriented session class which is compatible with PHP's session handling system. The object would read a given session file directly, unserialize it, provide access to the data via get/set methods, then then reserialize it and store it back to the file. Obviously if memcached sessions were enabled, it would use memcached as a backend instead of files.

Then the next step (maybe done by someone else) would be to use the new class for all session access, replacing $_SESSION.

Session files really are just files, and they're required to be writable by the web user so there's no problem with writing to them directly. The only tricky thing about it is the need to simulate the session-specific serialization function. For some reason, the session extension has its own format, and it doesn't provide a sensible interface to it.

#Comment by Brion VIBBER (talk | contribs)   00:58, 2 June 2011

I'm not really sure background download handling needs access to session data; my own inclination would be to have a table listing ongoing download jobs, which can be indexed by a token returned to the uploader.

Keeping it separate from the HTTP login session would also allow a separate login session by the same user to ping for status checks, which certainly seems plausible for huge-file transfers.

#Comment by MarkAHershberger (talk | contribs)   18:19, 12 June 2011

Totally agree. It seems like this was an abuse of session data to avoid having to figure out another way to track the background jobs.

#Comment by Bryan (talk | contribs)   21:58, 12 June 2011

So, what is your suggestion? Kill background uploading until somebody writes a way for jobs to communicate back? I'm fine with that.

#Comment by Brion VIBBER (talk | contribs)   19:29, 13 June 2011

Async uploading has been disabled for a couple versions already hasn't it?

#Comment by Bryan (talk | contribs)   21:33, 15 June 2011

It has been killed in 1.17. It can be killed from 1.18 and trunk as well if nobody bothers to write infrastructure to communicate back information from jobs. For this specific purpose though Neil's database backed upload stash could be used probably.

I don't have time to work on extensive projects though, so unless somebody else is willing to pick it up I think it should be removed from MediaWiki all together.

Status & tagging log