Pave the way for pluggable sessions handling and sessions caching
| Project: | Drupal |
| Version: | x.y.z |
| Component: | user system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | moshe weitzman |
| Status: | closed |
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.
| Attachment | Size |
|---|---|
| pluggablesessions.patch | 5.69 KB |
#1
rm extraneous ''
#2
Hmmm.... maybe the '' wasn't so extraneous:
<?phpfunction 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.
#3
<?phpfunction 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?
<?phpfunction drupal_count_sessions($timestamp = 0, $anonymous = true) {
$query = ($anonymous) ? ' AND uid = 0' : ' AND uid > 0';
...
?>
#4
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).
#5
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).
#6
Ooops, I'd introduced a bug in the query that gets the info for online authenticated users.
#7
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.
#8
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?
#9
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.
#10
Rerolled to track HEAD; followed all of Drumm's suggestions.
#11
Changed syntax for the parameter checking to something less verbose.
#12
#13
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.
#14
oops. proper patch here.
#15
oy. some .brzignore cruft in the last one. better one attached.
#16
(Please don't commit this yet. Want to review/test it first.)
#17
Also, please share performance results if possible.
#18
#19
@Dries - any chance you can benchmark this on your rig? High volume sites could really use this.
#20
rerolled for HEAD
#21
What testing steps do you want?
If you outline the steps we maybe able to deploy on a hardware cluster and test it.
Kieran
#22
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.
#23
No longer applies.
#24
drupal_count_sessions is broken. $timestamp parameter vs. $time_period used.
Unnecessary parentheses in $query definition.
+1 for pluggable session handling.
#25
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.
#26
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'.
#27
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?
#28
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.
#29
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.
#30
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".
#31
Why the second paramater to sess_destroy()? Type is always $uid so it is not actually needed. Don't complicate this patch. ;-)
#32
you don't see the need for ending a session based on SID?
#33
without the second parameter to sess_destroy.
#34
no, this is broken. Don't commit.
#35
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.
#36
patch
#37
Looking for reviewers.
#38
<?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.
#39
ok
#40
#41
Committed to CVS HEAD. Thanks.
#42
there's a misnamed variable in the last version.
#43
and unrelated to the bug in the previous followup, here is a patch against 4.7 in case anybody is interested.
#44
Committed to CVS HEAD. Thanks.
#45
#46
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.