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

StatusFileSize
new1.1 KB

Patch for 4.7.7.

Version:5.2» 6.0-beta1

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

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.

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.

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

Thanks, committed to D6.

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

Uh, patch is ready for D5.

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

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.

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.

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

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.

StatusFileSize
new1.42 KB

Implemented Dries' notes.

StatusFileSize
new1.88 KB

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

Status:Needs review» Reviewed & tested by the community

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

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.

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

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

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

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

Nitpicking...

StatusFileSize
new20.54 KB

Revised nitpicking...

StatusFileSize
new28.37 KB

Even more nitpicking... thanks to dmitri for reviewing

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

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

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.

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.

Status:Reviewed & tested by the community» Fixed

Committed to 5.x.

Status:Fixed» Closed (fixed)