I recently wanted to introduce memcached to a site and cache the sessions. This turned out to be slightly more difficult than expected because there is sessions handling code in user.module. This patch refactors that code out of user.module and puts it all in session.inc, where it belongs.

The advantages of doing this are many. First, we can easily switch our method of handling sessions by loading a different session.inc file (like chx's memcached_session.inc) during bootstrap. This makes session handling pluggable and will be a great asset to sites that want to manage sessions in-memory (like Digg and NowPublic). A second advantage is that the functionality of counting online users (formerly coded into the online users block of user.module) is now available as an API: drupal_count_sessions, and supports the counting of anonymous, authenticated or both.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

robertDouglass’s picture

FileSize
5.66 KB

rm extraneous ''

robertDouglass’s picture

Hmmm.... maybe the '' wasn't so extraneous:

function sess_destroy($key, $type = 'sid') {
  db_query("DELETE FROM {sessions} WHERE %s = '%s'", $type, $key);
}

Does this open us up to SQL injection? Then use the first patch.

robertDouglass’s picture

function drupal_count_sessions($timestamp = 0, $anonymous = 0) {
  switch ($anonymous) {
    case 0:
      $query =  ' AND uid = 0';
      break;
    case 1:
      $query = ' AND uid > 0';
      break;
    default:
      $query = '';
  }

Does anyone else find it counter-intuitive if $anonymous = 0? Would the function feel better like this?

function drupal_count_sessions($timestamp = 0, $anonymous = true) {
  $query = ($anonymous) ? ' AND uid = 0' : ' AND uid > 0';
  ...
robertDouglass’s picture

FileSize
5.67 KB

I like this one better. Addresses both of the above concerns. (note: the first patch doesn't work due to the '%s' = '%s' construction, so to avoid SQL injection I introduced my own validation on the parameter).

robertDouglass’s picture

FileSize
5.68 KB

Use true and false instead of 0 and 1 for better clarity (it is too psychologically confusing since we're talking about whether or not to query for uid = 0).

robertDouglass’s picture

FileSize
5.8 KB

Ooops, I'd introduced a bug in the query that gets the info for online authenticated users.

chx’s picture

sess_destroy IMO is conceptually wrong. Unless PHP calls it with random second parameters just document the valid chooses and remove that switch. Drupal sees such constructs as unnecessary cruft. Yes, if you call that function with the wrong parameters, then the program dies. So what? Don't call with wrong parameters.

robertDouglass’s picture

the switch is not to keep you from calling the function with a wrong second parameter, it is to allow me to safely put that parameter in the query without '', which is our protection against SQL injection. Calling the function without the switch and with SQL injection in the 2nd parameter is what I was worried about; maybe I'm misplacing my concern?

drumm’s picture

Status: Needs review » Needs work

Add a code comment to document that last follow-up. And i'm not sure we want to use %s for that, I think string concatenation is fine in that case.

robertDouglass’s picture

FileSize
5.97 KB

Rerolled to track HEAD; followed all of Drumm's suggestions.

robertDouglass’s picture

FileSize
5.92 KB

Changed syntax for the parameter checking to something less verbose.

robertDouglass’s picture

Status: Needs work » Needs review
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
37.54 KB

i improved docs a bit and add made throttle.module use the new drupal_count_sessions(). i grepped and noone else is touching sessions table anymore.

i tested the patch and seems fine.

incidentally, it would be very good to find a different way to count anon users so we didn't have to fill up session table with their records. both user block and throttle module currently use this info.

moshe weitzman’s picture

FileSize
6.44 KB

oops. proper patch here.

moshe weitzman’s picture

FileSize
6.34 KB

oy. some .brzignore cruft in the last one. better one attached.

Dries’s picture

(Please don't commit this yet. Want to review/test it first.)

Dries’s picture

Also, please share performance results if possible.

drumm’s picture

Status: Reviewed & tested by the community » Needs review
moshe weitzman’s picture

@Dries - any chance you can benchmark this on your rig? High volume sites could really use this.

moshe weitzman’s picture

FileSize
6.42 KB

rerolled for HEAD

Amazon’s picture

What testing steps do you want?

If you outline the steps we maybe able to deploy on a hardware cluster and test it.

Kieran

robertDouglass’s picture

This patch changes almost no logic... it just moves some code around.

To test you would make sure that people can log in, log out; that stuff that goes into their session stays in their session (such as comment format preferences). You would test that the number of authenticated and anonymous users in the "Who's online" block is accurate.

There is no need to test performance because there is *no* performance gain implicit in this patch. However, since it moves all of the session logic to one file, it is now possible to easily swap that file out with one that handles sessions totally differently, with memcached or LDAP or whatever.

Dries’s picture

Status: Needs review » Needs work

No longer applies.

jvandyk’s picture

drupal_count_sessions is broken. $timestamp parameter vs. $time_period used.

Unnecessary parentheses in $query definition.

+1 for pluggable session handling.

moshe weitzman’s picture

Assigned: Unassigned » moshe weitzman
Status: Needs work » Reviewed & tested by the community
FileSize
6.37 KB

fixed issues reported by JVD. i tested logout/login and who's online block. looks good.

as robert said, this patch just moves code around, and has no impact on performance.

Dries’s picture

Any performance results that back up the need for this, or that demonstrate the performance gain of alternative session mechanisms? While I believe that it can be useful, I'd be interested in those. I'm a curious person. :)

Anyway, I think that drupal_count_sessions should be called sess_count (for consistency). Oh, and I wouldn't mind a s/sess_/session_/cg after that. We don't abbreviate words like 'sessions'.

robertDouglass’s picture

I think session_destroy and so forth are off limits as they're already built-in php functions. I could make drupal_count_sessions into sess_count, or I could rename all the redefined session functions drupal_session_*. Do you have a preference?

robertDouglass’s picture

FileSize
7.06 KB

sess_count it is. I also discovered that the query on the users table to count authenticated users was wrong because it doesn't address authenticated users who log off. Now both anonymous and authenticated users are counted using sess_count, which is the way it was intended. I also renamed the variables in user.module to $authenticated and $anonymous to better reflect what it is they do.

robertDouglass’s picture

ok I broke the users list. Going back to fix. The sad truth is, we can't count authenticated users from the users table... the only accurate counting is the sess_count. That means that the list of online users is also inaccurate and doesn't account for users who just logged out.

robertDouglass’s picture

FileSize
7.41 KB

now we do both sess_count for authenticated users and the query on the users table to get the users' data. The count is accurate but the list may not be. We could fix this by changing the wording from "Online users" to "Recently seen".

Dries’s picture

Why the second paramater to sess_destroy()? Type is always $uid so it is not actually needed. Don't complicate this patch. ;-)

robertDouglass’s picture

you don't see the need for ending a session based on SID?

robertDouglass’s picture

FileSize
7.04 KB

without the second parameter to sess_destroy.

robertDouglass’s picture

no, this is broken. Don't commit.

robertDouglass’s picture

replaced a call to session_destroy (the php function) in user_logout. It was calling sess_destroy with the SID. Now we always call sess_destroy directly with $uid, simplifying the matter greatly.

robertDouglass’s picture

FileSize
7.25 KB

patch

robertDouglass’s picture

Status: Reviewed & tested by the community » Needs review

Looking for reviewers.

Dries’s picture

+  $result = db_fetch_object(db_query('SELECT COUNT(sid) AS count FROM {sessions} WHERE timestamp >= %d'. $query, $timestamp));
+  return $result->count;

can be:

+  return db_result(db_query('SELECT COUNT(sid) AS count FROM {sessions} WHERE timestamp >= %d'. $query, $timestamp));

Add spaces around '-':

... time()-$time_period ...
... time() - $time_period ...

Otherwise looks good.

robertDouglass’s picture

FileSize
7.22 KB

ok

robertDouglass’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

robertDouglass’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
693 bytes

there's a misnamed variable in the last version.

robertDouglass’s picture

and unrelated to the bug in the previous followup, here is a patch against 4.7 in case anybody is interested.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)
moshe weitzman’s picture

we never edited bootstrap.inc so that it is possible to use other session.inc like we now offer for cache.inc. so our CHANGELOG is a lie: "pluggable session handler ..."

help please.

fgm’s picture

Version: x.y.z » 4.7.9

bootstrap.inc was fixed in D5 (v1.145) to use the session_inc variable for this. So 4.7.9 was the last version for which this was a lie.