Hi y'all,
I've been developing a site on a system with a heavily loaded database server and I stumbled upon a race condition where messages added in a *_form_submit() hook by drupal_set_message() were not displaying on the redirected page. As it turned out session data was not written to the database before the browser received the redirection header and requested the new page.
I've made a patch for 4.7.7 and 5.2/HEAD (works on both). The intention of the patch is to move the call of header() in drupal_goto() to execute after the call to session_write_close(). This way session data is written to the database before the data is read again on the subsequent request.
FYI: register_shutdown_function() can accept parameters to the function as of PHP 4. Should be all good for minimum requirements of Drupal. Ref: http://www.php.net/manual/en/function.register-shutdown-function.php
Please review.
--
Sammy Spets
Synerger
Synerger Pty Ltd
Comment | File | Size | Author |
---|---|---|---|
#24 | session_write_before_goto_1.patch | 1.35 KB | JohnAlbin |
#22 | drupal-HEAD.nitpicking_2.patch | 28.38 KB | sun |
#21 | drupal-HEAD.nitpicking_1.patch | 28.37 KB | sun |
#20 | drupal-HEAD.nitpicking_0.patch | 20.54 KB | sun |
#19 | drupal-HEAD.nitpicking.patch | 20.5 KB | sun |
Comments
Comment #1
sammys CreditAttribution: sammys commentedPatch for 4.7.7.
Comment #2
sammys CreditAttribution: sammys commentedFlagging it for D6 instead so it'll get attention.
Comment #3
ilo CreditAttribution: ilo commentedI'm not sure if it can be considered "race condition" or "expected behaviour", as drupal_goto is expected to fresh going the requested end-point, and you may return from the submit with an end-point of the form, and messages will be displayed.
So goin to the destination using the return of the form is a better way than using drupal_goto.. I guess that was the purpose of introducing the form return value.
Comment #4
chx CreditAttribution: chx commentedsession.inc registers session_write_close first, this patch gets the redirect second. The D5 patch in the original issue applies cleanly to D6 as well.
Comment #5
Gábor HojtsyThanks, committed to D6.
Comment #6
Gábor HojtsyUh, patch is ready for D5.
Comment #7
JohnAlbinThis is causing a white screen when
drupal_goto()
is called under PHP 4.4.7. http://drupal.org/node/180644Comment #8
sunyes, specifically because of http://www.php.net/manual/en/function.register-shutdown-function.php :
Comment #9
JohnAlbinWatchdog is reporting:
And as the PHP docs say, you can’t modify the headers in shutdown function. Looks like we need to revert this change.
Also, here’s a suggested solution to the initial problem: Modify
drupal_goto()
to callsession_write_close()
before a call toheader("Location: ...")
. The subsequent, extraneous call tosession_write_close()
inside the shutdown function shouldn’t hurt anything.Comment #10
JohnAlbinThe previous patch doesn't REVERSE cleanly, so here’s an updated patch that reverses and adds
session_write_close()
beforedrupal_goto()
. I can split it into two patches if you like.I’ve verified that no PHP errors, PHP warnings or Drupal watchdog messages are caused by running
session_write_close()
twice.However, since I’m not experiencing the race condition, I can only say “I know this will fix the problem, but I haven’t tested it.”
Comment #11
Dries CreditAttribution: Dries commented1. "We need all session data to be written to the database before the header is sent to the browser."
For future reference, I think we'll want to extend the PHPdoc with one line. That line should mention why this is required.
2. "... REDIRECT status code to the http daemon"
REDIRECT should be redirect and http should be HTTP.
3. The documentation lines wrap at inconsistent lengths.
Comment #12
sunImplemented Dries' notes.
Comment #13
JohnAlbinAdded PHPDoc note and straightened out comments (which were originally taken directly from the old code).
Comment #14
sun#12 or #13 is ready to go, choose the one you like more.
Comment #15
JirkaRybka CreditAttribution: JirkaRybka commentedI confirm that drupal_goto() gives just blank screen and no redirect (php 4.4.4). I tested #13 patch, and it works fine.
I consider this as very critical bug, because now two subsequent 6.x-dev tarballs are horribly broken (every single form submission I tested ends up with white page - drupal_goto() is widely used function), so currently it makes other patches' (or any D6) testing a pain, and update.php (batch processing) doesn't work at all.
Comment #16
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #17
sammys CreditAttribution: sammys commentedThe call to session_write_close() is a great way to solve the problem. Nice work everyone.
Comment #18
sammys CreditAttribution: sammys commentedJust looked at the comments in the patch and "addition" has a typo.
Comment #19
sunNitpicking...
Comment #20
sunRevised nitpicking...
Comment #21
sunEven more nitpicking... thanks to dmitri for reviewing
Comment #22
sunFinal version. Thanks to all nitpickers who reviewed this! :-D
Comment #23
Gábor HojtsyWow, that was a huge docs cleanup patch. I read through the improvements and all seemed to be fine, so committed. Please open new issues with such huge docs updates next time. It helps keep different issues separated. Thanks.
Comment #24
JohnAlbinRe-rolled for 5.x. I didn’t include all the doc changes, just the bare minimum to fix this issue.
Comment #25
drummCommitted to 5.x.
Comment #26
(not verified) CreditAttribution: commented