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

Comments

sammys’s picture

StatusFileSize
new1.1 KB

Patch for 4.7.7.

sammys’s picture

Version:5.2» 6.0-beta1

Flagging it for D6 instead so it'll get attention.

ilo’s picture

I'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.

chx’s picture

Status:Needs review» Reviewed & tested by the community

Multiple calls to register_shutdown_function() can be made, and each will be called in the same order as they were registered. If you call exit() within one registered shutdown function, processing will stop completely and no other registered shutdown functions will be called.

session.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.

Gábor Hojtsy’s picture

Version:6.0-beta1» 5.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks, committed to D6.

Gábor Hojtsy’s picture

Status:Patch (to be ported)» Reviewed & tested by the community

Uh, patch is ready for D5.

JohnAlbin’s picture

Version:5.x-dev» 6.x-dev
Status:Reviewed & tested by the community» Needs work

This is causing a white screen when drupal_goto() is called under PHP 4.4.7. http://drupal.org/node/180644

sun’s picture

yes, specifically because of http://www.php.net/manual/en/function.register-shutdown-function.php :

Note: Shutdown function is called during the script shutdown so headers are always already sent.

JohnAlbin’s picture

Watchdog is reporting:

Cannot modify header information - headers already sent in Unknown on line 0.

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 call session_write_close() before a call to header("Location: ..."). The subsequent, extraneous call to session_write_close() inside the shutdown function shouldn’t hurt anything.

JohnAlbin’s picture

Assigned:sammys» JohnAlbin
Status:Needs work» Needs review
StatusFileSize
new1.36 KB

The previous patch doesn't REVERSE cleanly, so here’s an updated patch that reverses and adds session_write_close() before drupal_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.”

Dries’s picture

1. "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.

sun’s picture

StatusFileSize
new1.42 KB

Implemented Dries' notes.

JohnAlbin’s picture

StatusFileSize
new1.88 KB

Added PHPDoc note and straightened out comments (which were originally taken directly from the old code).

sun’s picture

Status:Needs review» Reviewed & tested by the community

#12 or #13 is ready to go, choose the one you like more.

JirkaRybka’s picture

I 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.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

sammys’s picture

The call to session_write_close() is a great way to solve the problem. Nice work everyone.

sammys’s picture

Just looked at the comments in the patch and "addition" has a typo.

sun’s picture

Assigned:JohnAlbin» sun
Priority:Critical» Minor
Status:Fixed» Needs review
StatusFileSize
new20.5 KB

Nitpicking...

sun’s picture

StatusFileSize
new20.54 KB

Revised nitpicking...

sun’s picture

StatusFileSize
new28.37 KB

Even more nitpicking... thanks to dmitri for reviewing

sun’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new28.38 KB

Final version. Thanks to all nitpickers who reviewed this! :-D

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Fixed

Wow, 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.

JohnAlbin’s picture

Version:6.x-dev» 5.x-dev
Status:Fixed» Reviewed & tested by the community
StatusFileSize
new1.35 KB

Re-rolled for 5.x. I didn’t include all the doc changes, just the bare minimum to fix this issue.

drumm’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 5.x.

Anonymous’s picture

Status:Fixed» Closed (fixed)