Last modified: 2009-07-28 17:43:22 UTC
Created attachment 6388 [details] Patch against r53755 With the standard php session handling, Http::update_session_progress and UploadBase::stashSession both screw everything up by calling session_write_close(). This doesn't happen with memcached sessions, because memcached sessions requires neither session_start() nor session_write_close() and in fact redefines these as do-nothing functions. Consider for example what happens when an API upload command is issued to synchronously download a file from a url and the upload returns warnings. 1. Http::update_session_progress is called during the handling of the download, which calls session_write_close. 2. UploadBase::stashSession is called so the upload can be completed later. $_SESSION is already set because of earlier session_start or wfSetupSession calls, so the session is never re-opened (and even if it would be called, would it actually work right?). 3. UploadBase::stashSession calls session_write_close, which doesn't do anything useful because there is no session open. 4. The page processing ends, and any changes to $_SESSION are again not written because no session is open. As far as I can tell, MediaWiki just isn't set up for closing the session; otherwise there would probably be a "wfCloseSession" function to match wfSetupSession. The attached patch tries to fix this issue by having UploadBase::stashSession not close the session and having Http::update_session_progress only close the session if it opened it in the first place. Whether that's really the best way to fix this issue, I don't know.
I think this might have been the same issue I had on bug 19754. I was never able to track down the root of the problem, but this does seem similar to what I observed.
Closing the session in UploadBase::stashSession is unnecessary since it one synchronous request. We can take that out done in r53791 For Http::update_session_progress we need to open/close the session to avoid "locking". If you could be more specific about "screw everything up" that would be helpful. I am running it locally with php based sessions and seems to work giving me status updates of http download progress. If don't close the session, the first status update just hangs waiting for the download to finish and close the session. The session will have always been started prior to running Http::update_session_progress by doSessionIdDownload
Using r53792 to test. I issue the following API command: maxlag=5&format=json&filename=Test.svg&action=upload&url=http://upload.wikimedia.org/wikipedia/commons/a/a4/Red_bug.svg&token=[...] I get back the following response: <br /> <b>Fatal error</b>: Call to a member function isOK() on a non-object in <b>/usr/local/src/MediaWiki/phase3/includes/api/ApiUpload.php</b> on line <b>132</b><br /> After applying my patch from bug 19930, I reissue the above command and get the expected warning response (too long to quote here, if you REALLY want it I can attach it) with a session key. Then I issue the following API command to try to complete the upload: maxlag=5&format=json&filename=Test.svg&ignorewarnings=1&action=upload&sessionkey=[...]&token=[...] I get back the following response: {"upload":{"result":"Failure","error":"unknown-error","code":{"status":3}}} After applying my patch from bug 19931 and re-running the queries, I get the following slightly more helpful result: {"upload":{"result":"Failure","error":"empty-file"}} When I examine the session file for that request in /var/lib/php5, I observe that it does NOT contain wsUploadData. The only keys contained are wsUserID, wsToken, wsUserName, wsEditToken, and wsDownload. When I apply the HttpFunctions.php part of the patch I attached to this bug and re-run the queries again, the upload succeeds. The session file for that request DOES, of course, contain the wsUploadData. It seems clear that Http::update_session_progress calling session_write_close is breaking session writing in any part of MediaWiki called after that function. I suppose that's not "everything", but it is most certainly screwing things up.
Thanks for the details. Your right. Http::update_session_progress should only be called when we issuing a background download process not in the case of stashing a file or other synchronous requests. Added a flag to not do "secession close" updates unless its called from the command line asynchronously. in r53812 There still seems to be a few some minor bugs in the upload flow around the warnings and js2 catching those warnings. Will follow up with another commit shortly
As of r53871, you're still calling update_session_progress from simpleFileWriter::close even when your new flag is set.
thanks for catching that, fixed in r53881
