Fix and streamline page cache and session handling

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.

+    session_id(md5(uniqid()));

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

+    // session is only started on demand in drupal_commit_session(), making

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:

function drupal_session_commit() {
  global $user;

  if (empty($user->uid) && empty($_SESSION)) {
    if (drupal_session_started()) {
      // Destroy empty anonymous sessions.
      session_destroy();
    }
  }
  else {
    if (!drupal_session_started()) {
      drupal_session_start();
    }
    session_write_close();
  }
}

+function drupal_session_start() {
+  global $user;

$user is not used in that function.

       if (!isset($_SESSION['openid'])) {
-        drupal_set_session('openid', array());
+        $_SESSION['openid'] = array();
       }
       $_SESSION['openid']['values'] = $form_state['values'];

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.

     $anonymous = drupal_session_count(0, TRUE);
     $authenticated = drupal_session_count(0, FALSE);
-    $this->assertEqual($anonymous + $authenticated, $this->session_count, t('Correctly counted @count total sessions.', array('@count' => $this->session_count)), t('Session'));
+    $this->assertEqual($anonymous + $authenticated, $session_count['anonymous'] + $session_count['authenticated'], t('Correctly counted @count total sessions.', array('@count' => 1)), t('Session'));

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

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

#0  drupal_session_start() called at [includes/session.inc:173]
#1  drupal_session_initialize() called at [includes/bootstrap.inc:1362]
#2  _drupal_bootstrap(4) called at [includes/bootstrap.inc:1298]
#3  drupal_bootstrap(9) called at [index.php:21]

Warning: session_start(): Cannot send session cache limiter - headers already sent (output started at includes/session.inc:196) in includes/session.inc on line 197

Warning: Cannot modify header information - headers already sent by (output started at includes/session.inc:196) in includes/bootstrap.inc on line 854

Warning: Cannot modify header information - headers already sent by (output started at includes/session.inc:196) in includes/bootstrap.inc on line 854

Warning: Cannot modify header information - headers already sent by (output started at includes/session.inc:196) in includes/bootstrap.inc on line 854

Warning: Cannot modify header information - headers already sent by (output started at includes/session.inc:196) in includes/bootstrap.inc on line 854
#0  drupal_session_start() called at [includes/session.inc:223]
#1  drupal_session_commit() called at [includes/common.inc:330]
#2  drupal_goto() called at [modules/user/user.pages.inc:146]
#3  user_logout()
#4  call_user_func_array(user_logout, Array ()) called at [includes/menu.inc:402]
#5  menu_execute_active_handler() called at [index.php:22]

Fatal error: session_start(): Failed to initialize storage module: user (path: /var/lib/php5) in includes/session.inc on line 197

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.

Fatal error: session_start(): Failed to initialize storage module: user (path: /Applications/MAMP/tmp/php) in /Users/webchick/Sites/core/includes/session.inc on line 196
Call Stack:
0.0006 72664 1. {main}() /Users/webchick/Sites/core/index.php:0
0.1544 9654224 2. menu_execute_active_handler() /Users/webchick/Sites/core/index.php:22
0.1580 9811928 3. call_user_func_array() /Users/webchick/Sites/core/includes/menu.inc:402
0.1580 9811928 4. user_logout() /Users/webchick/Sites/core/includes/menu.inc:0 
0.1649 9827136 5. drupal_goto() /Users/webchick/Sites/core/modules/user/user.pages.inc:146
0.1650 9827544 6. drupal_session_commit() /Users/webchick/Sites/core/includes/common.inc:330
0.1651 9827732 7. drupal_session_start() /Users/webchick/Sites/core/includes/session.inc:222
0.1651 9827988 8. session_start() /Users/webchick/Sites/core/includes/session.inc:196

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.

@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:

+  // Destroy the current session.
   session_destroy();
+  $_SESSION = array();

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?

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.

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

 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.

+    $this->pass($this->drupalGetHeader('Set-Cookie'));

A left-over from debugging?

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

+    // 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().

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

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?

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

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.

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

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()...

changing tagging scheme for update guide stuff.

sun: does this need other documentation work besides the update guide?

Yes - at least I don't think that the new session handling with regard to anonymous users is documented anywhere.

The only documentation I was able to find is @file-level in session.inc:
http://api.drupal.org/api/drupal/includes--session.inc/7

Previously, all of this didn't require many details that one should be aware of. But starting with D7, it highly matters that developers know that putting stuff into $_SESSION has a large impact on performance, and they also need to know, how the emptiness of $_SESSION is evaluated.

OK. Adding back the generic "needs documentation" tag, since this needs regular doc as well as an entry in the update modules 67/ page.

subscribe

An issue summary on this issue would surely help.... sigh.

So. It looks like patches were committed from #11
http://drupal.org/files/issues/477944-fix-streamline-session-page-cache_...
and #46
http://drupal.org/files/issues/477944-follow-up_4.patch

API changes I am seeing:
a) Function name changes - _sess_write() -> drupal_session_write() etc.
This is already documented on the update guide:
http://drupal.org/node/224333#session_rename

b) Some logic in drupal_session_write() moved into drupal_save_session() (to avoid saving anon sessions). I don't know that this particularly needs to be documented.

c) Session destroying is more careful (some things previously done in user_logout() are moved into session_destroy). Again, I'm not sure why we need to document this. There are a few other cases like (b) and (c) too that I don't think deserve a specific mention.

d) function page_get_cache -> drupal_page_get_cache. That needs to be documented on the 6/7 update guide.

e) function drupal_page_cache_header -> drupal_serve_page_from_cache(). That needs to be documented in the 6/7 update guide.

f) Various modules are using different keys in $_SESSIONS than they used to, but I don't think that's worth documenting in the module update guide probably?

So, the items that I think need mentioning in the update guide are (d) and (e). And also see comment #49 for another thing in the update guide that needs to be fixed.

As far as the other doc issues (issues with the API doc), I think we should file a separate issue rather than following up here. So, although I don't necessary understand much about sessions, I took the above comments and filed:
#1188574: session API function doc is lacking
Therefore, clearing the "needs doc" tag, since that is on its own issue now.

I've added doc of (d) and (e) to the 6/7 module page:
http://drupal.org/update/modules/6/7#page-cache-rename

Also I added comments to the 6/7 functions to link them to each other on api.drupal.org. I think this is sufficiently fixed now.

I'm stumbling on a problem with the mandatory httpOnly cookie setting on D8 (accessing the session cookie from a JS-only app). This settings appears to have been added by Damien without further discussion : what was the reason behind forcing it instead of allowing it to be changed in the application settings , apart from the general observations as on https://www.owasp.org/index.php/HttpOnly ? This has been carried identically on D8.