Fix and streamline page cache and session handling

Using drupal_static() instead of the static in drupal_page_cacheable(), thanks chx.

I think this is very very nice. Biggest positives are the removal of drupal_set_session and the anonymous session destroy. Once this is in, we can get back to the page cache simplify which will be a LOT easier to accomplish after this.

Very nice work.

uniqid() is slow, uniqid(TRUE) is much faster (see #251792: Implement a locking framework for long operations, comments #79 and #81).

The function name is drupal_session_commit().

AFAICT drupal_session_commit() can be written in a more compact and readable way with identical behaviour like this:

$user is not used in that function.

You might want to skip the whole if-block. It was introduces with this patch in #201122: Drupal should support disabling anonymous sessions. This pattern occurs several places.

Why are $session_count['anonymous'] and $session_count['authenticated'] stored in an array, when $anonymous and $authenticated are not? I suggest eliminating the array, unless there is a special reason for using it that I've missed.

During testing I got the following error, but now I cannot reproduce it, so it may be caused by something else: Fatal error: session_start(): Failed to initialize storage module: user (path: /tmp) in /home/chsc/www/drupal7/includes/session.inc on line 196
#0 drupal_session_start() called at [/home/chsc/www/drupal7/includes/session.inc:172]
#1 drupal_session_initialize() called at [/home/chsc/www/drupal7/includes/bootstrap.inc:1366]
#2 _drupal_bootstrap(4) called at [/home/chsc/www/drupal7/includes/bootstrap.inc:1302]
#3 drupal_bootstrap(9) called at [/home/chsc/www/drupal7/index.php:21]

I wonder whether it would be more efficient to replace print drupal_render_page($return); in index.php with $output =  drupal_render_page($return); and then pass that to drupal_page_footer(), and then add ob_end_flush(); print $output to the bottom of drupal_page_footer(). The whole page has already been generated in one string, and this change prevents the string from being put into the output buffer before being sent to the client. I haven't benchmarked it, though. The same approach can be used by drupal_serve_page_from_cache().

I saw this error message too (but wasn't able to reproduce it either), and it seems related to: PHP Bug #45363. My gut feeling is that moving the session_set_save_handler() just before session_start() should prevent the issue from happening.

Here is a new version, taking care of c960657 remarks:
- use uniqid('', TRUE) instead of uniqid(), which is slower on some systems
- fix comments
- refactor drupal_session_commit() for clearness
- removed a useless $user
- revert useless isset() constructs all over the place
- remove the array in session.test
- move session_set_save_handler() just before session_start()

I haven't made the suggested change to how we handle output buffers. I don't think it would make a big difference, and it can wait for another patch!

The last submitted patch failed testing.

Erm.

Calling session_set_save_handler() apparently resets the session handler state, for some (internal PHP) reasons. We cannot move session_set_save_handler() into drupal_session_start(), because we already called some session_* functions at this time. This patch should pass the test bot.

The last submitted patch failed testing.

I accidentally introduced a few E_ALL warnings. Lets fix those.

I gave this patch another good look, and it is a very solid clean-up. Committed to CVS HEAD so we can keep making progress. Great work!

I still get this error when logging out (using PHP 5.2.0):
Fatal error: session_start(): Failed to initialize storage module: user (path: /tmp) in /home/chsc/www/drupal7/includes/session.inc on line 196

I can confirm the fatal error on a clean install of HEAD on a clean db.

For whatever reason, the only page that produces this error for me is the Clean URLs settings page. To produce:

1. Log into admin.
2. Optionally: visit whatever pages you want.
3. Visit the clean urls page, admin/settings/clean-urls.
4. Optionally: visit whatever pages you want.
5. Log out.

And I get that php error.

Reverting to the June 2, 2009 revision (i.e., -D "2 June 2009") removes the problem.

Test server has been removed.
Since some people had problems reproducing the error, I've set up a test server for this:
http://477944.jamesan.ca
User: 477944
Pass: 477944
There's a primary link to the Clean URLs settings page at the top-right corner of the page.

I've pinpointed the problem to be something to do with assigning a value in to the $_SESSION in system_clean_url_settings():
$_SESSION['clean_url'] = TRUE;.

We can replace the use of $_SESSION['clean_url'] with the variable_get() and variable_set() with 'clean_url', which is already used to store whether clean urls are enabled.

The patch makes that change, although I don't think this fix addresses the real problem.

That sounds like a race condition somewhere. That would explain why it is hard to reproduce.

Here's the PHP stack trace before the fatal error:

It's not hard to reproduce, it just so happens to happen *exactly once* on a fresh install. JamesAn nails it exactly in #14. You MUST start with a fresh install. Immediately after finishing the install, try to logout. Drupal throws the session error but does actually log you out. Refresh the page and see that you're now logged out. After the first time logging out, you can login/logout without any further trouble.

I confirm I'm getting the same thing, somewhat randomly. quicksketch sees it too.

Info from XDebug which looks pretty identical to what JamesAn posted.

Oh, slow on the draw. :)

What happens, I think, is this:

1. The user has a non-empty $_SESSION and logs out.
2. user_logout() calls session_destroy(). This doesn't reset $_SESSION as one might assume, but the contents are discarded anyway due to the Fatal error occurring before the session data has been saved to the anonymous user's session.
3. user_logout() then calls drupal_goto() that calls drupal_session_commit().
4. due to the non-empty $_SESSION, drupal_session_commit() calls drupal_session_start() that calls session_start(), and that fails due to the mentioned PHP bug.

The test actually triggers the bug, but it isn't noticed because DrupalWebTestCase::drupalLogout() simply discards the output from user/logout.

This patch makes user_logout() reset $_SESSION. Also it was necessary to reset $this->loggedInUser when changing session cookies in order to prevent drupalLogin() from calling drupalLogout() first, even if the new session cookie did not represent a logged in user. The patch doesn't go further to workaround the PHP bug - I'm not sure whether this is necessary.

The problem seems fixed when I apply this patch. Hopefully testbot will come back with happy results.

Nice catch, c960657. I wasn't able to understand the scenario that made us start a session twice.

@JamesAn: does that solves the issue you were seeing on the clean-url page?

@Damien Tournoud: yes, indeed.

At the very least we should document why session_destroy() is not doing what it says on the tin. This part:

But it feels like we're only masking something deeper.

Doing a bit of digging, apparently session_destroy() does not unset $_SESSION.

From http://ca.php.net/session_destroy

[session_destroy()] does not unset any of the global variables associated with the session, or unset the session cookie.

and again: Johan 20-Nov-2004 02:00

Remember that session_destroy() does not unset $_SESSION at the moment it is executed. $_SESSION is unset when the current script has stopped running.

So we do need to manually clear $_SESSION like it's done in #21. We can't unset it "as this will disable the registering of session variables through the $_SESSION superglobal." (http://ca2.php.net/manual/en/function.session-unset.php)

Should we document that session_destroy() doesn't clear $_SESSION, since it's not obvious?

Ok, well then my question is, "Is this actually desirable?" :) Because that $_SESSION = array() line has not existed for the entire history of Drupal (or at least since 4.6) in that function. Why do we need to add it now?

Ok, apparently this is not very clear:

• Calling session_destroy() does destroy all session data. PHP calls the "destroy" session handler (in our case http://api.drupal.org/api/function/_sess_destroy_sid/7), so the user session data is completely lost. What PHP doesn't do, in its silliness, is (1) destroy the session cookie and (2) destroy the session data that is stored, in memory, inside $_SESSION.
• Because $_SESSION is not destroyed, our "smart" session handler will see that (a) the current user has no session (of course, it was killed by session_destroy()!) and (b) that there are data in $_SESSION. As a consequence, it will try to create a session for that user, and miserably fail because of the PHP bug identified earlier.

Because we don't want to restart a session in that case, the correct fix is indeed to nuke $_SESSION data.

Oops. Forgot that Drupal changes the session storage functions.

The bug seems to only rear its head when code manually sets $_SESSION keys, like the clean url settings page. Perhaps we haven't seen it before as modules have historically not fiddled around in $_SESSION, as $_SESSION values are transient and settings are permanently saved in the db.

Does this need to be backported?

This needs to be better documented in the code, IMO.

Added a comment above the resetting of $_SESSION.

The comment works for me. I think this patch is good to go since it's fixed the problem on my test.

Further clean-up (and an additional test): we don't want to start a session when drupal_save_session() is FALSE.

I moved cleaning $_SESSION and unsetting the cookie in _sess_destroy_sid(), where it makes more sense.

This looks good. I like that more logic is moved from the user module into the session handler.

Isn't this condition always TRUE? If yes, we might as well omit it. If not, a clarifying comment would be nice.

A left-over from debugging?

In drupal_session_commit(), should we - for symmetry - avoid destroying empty anonymous sessions, if drupal_save_session() is FALSE?

The word “restore” gives me the impression that $user was somehow reset and needs to be restored. Perhaps something like this instead: Reset $_SESSION and $user to prevent a new session from being started in drupal_session_commit().

Now that you are touching this line, please change time() to REQUEST_TIME.

For anyone on the infrastructure team looking at this issue, the only commit to HEAD referencing this issue is this:

http://vcs.fourkitchens.com/drupal/7-all-history/revision/10220

Does anyone know if any of the cleanup patches have made it in?

Here is an updated patch.

function _sess_destroy_sid($sid) {
...
+ if ($sid == session_id()) {
Isn't this condition always TRUE? If yes, we might as well omit it. If not, a clarifying comment would be nice.

No. This function can be called for a session ID that is not the one of the current user. Added a comment.

+ $this->pass($this->drupalGetHeader('Set-Cookie'));
A left-over from debugging?

Removed.

In drupal_session_commit(), should we - for symmetry - avoid destroying empty anonymous sessions, if drupal_save_session() is FALSE?

Indeed. Fixed.

+ // Remove $_SESSION data, and restore the user object.
+ $_SESSION = array();
+ $user = drupal_anonymous_user();
The word “restore” gives me the impression that $user was somehow reset and needs to be restored. Perhaps something like this instead: Reset $_SESSION and $user to prevent a new session from being started in drupal_session_commit().

Ok.

+ setcookie(session_name(), '', time() - 3600, $params['path'], $params['domain'], $params['secure'], $params['httponly']);
Now that you are touching this line, please change time() to REQUEST_TIME.

Done.

"Always use httponly cookies." is an inaccurate comment. That configuration only changes the HTTP-only status for *session* cookies.

"Destroy the current session, it will also reload the anonymous user." is a comma splice.

"Destroy the current session, it will also reset $user to the anonymous
+ // user." is also a comma splice.

Change both comma splice issues to "Destroy the current session and reset $user to the anonymous user."

Done.

Better yet.

Looks good to me.

No. This function can be called for a session ID that is not the one of the current user. Added a comment.

When is the function called with a session other than the one of the current user? It isn't documented explicitly in the PHP manual for session_set_save_handler() that this wont happen, but _sess_read() and _sess_write() assume this (they use the global $user object). So I think _sess_destroy_sid() should make the same assumption, unless there is a reason not to do so.

(For further consistency, _sess_destroy_sid() should probably be renamed to just _sess_destroy(), and $key should be renamed to $sid a few places in session.inc, but we can do that in another issue)

I think the flow in drupal_session_commit() would be easier to follow if drupal_save_session() is called only once in the benning of the function, i.e. if (!drupal_save_session()) { return; } .

+ // Reset $_SESSION and $user to prevent a new session from being started
+ // in drupal_session_commit()
Sentence does not end with a period.

Subscribing. Berdir adviced me to look at this issue since this patch might also solve #500680: Fatal error on logout. I will test is whenever I find the time to do it.

Marked #500680: Fatal error on logout as duplicate. The current issue will indeed solve this bug if the session data are destroyed when the session is destroyed. For the moment, I only tested Damien Tournoud's patch to see if it would solve the bug I spotted and it does solve it.

Here are further cleanups following #41:

- _sess_*() are now renamed _drupal_session_*(). We don't do abbreviations, and those were hurting my eyes.
- $key is now consistently $sid
- _drupal_session_destroy() now assume that $sid is the one of the user (I agree with c960657 that it makes sense)
- refactored drupal_session_commit() code flow so that we only call drupal_save_session() once, and added a few comments to ease readability

Untested patch, but the test bot is there for us, right? ;)

The last submitted patch failed testing.

And a non broken one.

Tests pass and it looks like a fairly straight-forward but important clean-up. Committed.

Automatically closed -- issue fixed for 2 weeks with no activity.

This is entirely undocumented. Worse, the upgrade guide still states drupal_set_session() in http://drupal.org/node/224333#drupal_set_session, which does not exist anymore.

I had to dig through 4-5 issues + look at each committed patch to find the one that is guilty.

Furthermore, the in-code API documentation is a bit sparse... At least drupal_session_start() should clarify Drupal's default session handling, IMHO. In addition to that, the first function that developers are looking at is drupal_session_initialize(), but that literally has zero documentation about what it does and what it doesn't do, neither any mentioning of drupal_session_start()...