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.
Comment | File | Size | Author |
---|---|---|---|
#43 | pluggablesessions47.patch.txt | 7.49 KB | robertDouglass |
#42 | user.patch_10.txt | 693 bytes | robertDouglass |
#39 | pluggablesessions_11.patch | 7.22 KB | robertDouglass |
#36 | pluggablesessions_10.patch | 7.25 KB | robertDouglass |
#33 | pluggablesessions_9.patch | 7.04 KB | robertDouglass |
Comments
Comment #1
robertDouglass CreditAttribution: robertDouglass commentedrm extraneous ''
Comment #2
robertDouglass CreditAttribution: robertDouglass commentedHmmm.... maybe the '' wasn't so extraneous:
Does this open us up to SQL injection? Then use the first patch.
Comment #3
robertDouglass CreditAttribution: robertDouglass commentedDoes anyone else find it counter-intuitive if $anonymous = 0? Would the function feel better like this?
Comment #4
robertDouglass CreditAttribution: robertDouglass commentedI 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).
Comment #5
robertDouglass CreditAttribution: robertDouglass commentedUse 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).
Comment #6
robertDouglass CreditAttribution: robertDouglass commentedOoops, I'd introduced a bug in the query that gets the info for online authenticated users.
Comment #7
chx CreditAttribution: chx commentedsess_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.
Comment #8
robertDouglass CreditAttribution: robertDouglass commentedthe 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?
Comment #9
drummAdd 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.
Comment #10
robertDouglass CreditAttribution: robertDouglass commentedRerolled to track HEAD; followed all of Drumm's suggestions.
Comment #11
robertDouglass CreditAttribution: robertDouglass commentedChanged syntax for the parameter checking to something less verbose.
Comment #12
robertDouglass CreditAttribution: robertDouglass commentedComment #13
moshe weitzman CreditAttribution: moshe weitzman commentedi 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.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedoops. proper patch here.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedoy. some .brzignore cruft in the last one. better one attached.
Comment #16
Dries CreditAttribution: Dries commented(Please don't commit this yet. Want to review/test it first.)
Comment #17
Dries CreditAttribution: Dries commentedAlso, please share performance results if possible.
Comment #18
drummComment #19
moshe weitzman CreditAttribution: moshe weitzman commented@Dries - any chance you can benchmark this on your rig? High volume sites could really use this.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedrerolled for HEAD
Comment #21
Amazon CreditAttribution: Amazon commentedWhat testing steps do you want?
If you outline the steps we maybe able to deploy on a hardware cluster and test it.
Kieran
Comment #22
robertDouglass CreditAttribution: robertDouglass commentedThis 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.
Comment #23
Dries CreditAttribution: Dries commentedNo longer applies.
Comment #24
jvandyk CreditAttribution: jvandyk commenteddrupal_count_sessions is broken. $timestamp parameter vs. $time_period used.
Unnecessary parentheses in $query definition.
+1 for pluggable session handling.
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedfixed 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.
Comment #26
Dries CreditAttribution: Dries commentedAny 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'.
Comment #27
robertDouglass CreditAttribution: robertDouglass commentedI 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?
Comment #28
robertDouglass CreditAttribution: robertDouglass commentedsess_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.
Comment #29
robertDouglass CreditAttribution: robertDouglass commentedok 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.
Comment #30
robertDouglass CreditAttribution: robertDouglass commentednow 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".
Comment #31
Dries CreditAttribution: Dries commentedWhy the second paramater to sess_destroy()? Type is always $uid so it is not actually needed. Don't complicate this patch. ;-)
Comment #32
robertDouglass CreditAttribution: robertDouglass commentedyou don't see the need for ending a session based on SID?
Comment #33
robertDouglass CreditAttribution: robertDouglass commentedwithout the second parameter to sess_destroy.
Comment #34
robertDouglass CreditAttribution: robertDouglass commentedno, this is broken. Don't commit.
Comment #35
robertDouglass CreditAttribution: robertDouglass commentedreplaced 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.
Comment #36
robertDouglass CreditAttribution: robertDouglass commentedpatch
Comment #37
robertDouglass CreditAttribution: robertDouglass commentedLooking for reviewers.
Comment #38
Dries CreditAttribution: Dries commentedcan be:
Add spaces around '-':
Otherwise looks good.
Comment #39
robertDouglass CreditAttribution: robertDouglass commentedok
Comment #40
robertDouglass CreditAttribution: robertDouglass commentedComment #41
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #42
robertDouglass CreditAttribution: robertDouglass commentedthere's a misnamed variable in the last version.
Comment #43
robertDouglass CreditAttribution: robertDouglass commentedand unrelated to the bug in the previous followup, here is a patch against 4.7 in case anybody is interested.
Comment #44
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #45
(not verified) CreditAttribution: commentedComment #46
moshe weitzman CreditAttribution: moshe weitzman commentedwe 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.
Comment #47
fgmbootstrap.inc
was fixed in D5 (v1.145) to use thesession_inc
variable for this. So 4.7.9 was the last version for which this was a lie.