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.

Comments

StatusFileSize
new5.66 KB

rm extraneous ''

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

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

<?php
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?

<?php
function drupal_count_sessions($timestamp = 0, $anonymous = true) {
 
$query = ($anonymous) ? ' AND uid = 0' : ' AND uid > 0';
  ...
?>

StatusFileSize
new5.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).

StatusFileSize
new5.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).

StatusFileSize
new5.8 KB

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

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.

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?

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.

StatusFileSize
new5.97 KB

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

StatusFileSize
new5.92 KB

Changed syntax for the parameter checking to something less verbose.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new37.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.

StatusFileSize
new6.44 KB

oops. proper patch here.

StatusFileSize
new6.34 KB

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

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

Also, please share performance results if possible.

Status:Reviewed & tested by the community» Needs review

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

StatusFileSize
new6.42 KB

rerolled for HEAD

What testing steps do you want?

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

Kieran

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.

Status:Needs review» Needs work

No longer applies.

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

Unnecessary parentheses in $query definition.

+1 for pluggable session handling.

Assigned:Unassigned» moshe weitzman
Status:Needs work» Reviewed & tested by the community
StatusFileSize
new6.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.

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

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?

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

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.

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

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

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

StatusFileSize
new7.04 KB

without the second parameter to sess_destroy.

no, this is broken. Don't commit.

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.

StatusFileSize
new7.25 KB

patch

Status:Reviewed & tested by the community» Needs review

Looking for reviewers.

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

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

Add spaces around '-':
<?php
... time()-$time_period ...
...
time() - $time_period ...
?>

Otherwise looks good.

StatusFileSize
new7.22 KB

ok

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Reviewed & tested by the community
StatusFileSize
new693 bytes

there's a misnamed variable in the last version.

StatusFileSize
new7.49 KB

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

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Closed (fixed)

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.

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.