The "Who's online" block shows a number of users inconsistent with the user list. For example '... currently 1 user ... online', but the user list shows 'userA, userB, ...' (see screenshot).
This is because the number of users is determined through sess_count() function which simply returns the number of rows in the 'session' table. The row is deleted when the user session ends. But the list of online users is collected through a query against 'users' table and returns all users where the 'access' timestamp is within a certain interval. The 'access' field is not changed when the users logs out.

CommentFileSizeAuthor
#20 inconsistent_whos online_message.png4.51 KBfazal_rkmp
#16 online_block_5.patch1.43 KBprofix898
#15 online_block_4.patch1.35 KBprofix898
#9 online_block_3.patch1.35 KBprofix898
#7 online_block_2.patch1.35 KBprofix898
#5 online_block_1.patch1.02 KBprofix898
#2 online_block_0.patch984 bytesprofix898
#1 online_block.patch805 bytesprofix898
online_block.png3.81 KBprofix898
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

profix898’s picture

profix898 CreditAttribution: profix898 commented
Status: Active » Needs review
FileSize
online_block.patch805 bytes

Here is a very simple patch. The number of online users is now quantified using db_num_rows() on the list of online (authenticated) users.

profix898’s picture

profix898 CreditAttribution: profix898 commented
FileSize
online_block_0.patch984 bytes

This patch also corrects the time_period, currently users are never removed from the list. The query checks for access time >= period (period = 900 by default) instead of access time >= time() - period.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

Works as expected. This also removes a query.

For those reviewing the patch and confused about the time() bug mentioned the time_period assignment helps explain it:

$time_period = time() - variable_get('user_block_seconds_online', 900);

Since we are already subtracting from time the query was asking for users who's last access was >= time() - time() - 900. That's clearly not correct :). Good catch profix898, everything looks in order.

Dries’s picture

Dries CreditAttribution: Dries commented

But isn't the implementation of sess_count() wrong than? (We use that function in a couple of places.)

profix898’s picture

profix898 CreditAttribution: profix898 commented
Status: Reviewed & tested by the community » Needs review
FileSize
online_block_1.patch1.02 KB

The rows in session table are removed the moment the users logs off. But in users table the access timestamp remains. We are running a query on sessions table to get the number of 'active' sessions, but for the user list we receive all entries from the users table where last access is within the time_period interval. What means in 4.7 and 5.0b1 users which logged out some time ago (within the intervall) are still listed, although their sessions entry was already removed.
IMO its is more consistent to remove the user from "Who's online" block the moment he/she logs out. This also removes the inconsistency between the online block and sess_count.

This patch looks up the uids of all (non-anonymous) sessions from sessions table (same as sess_count) and uses INNER JOIN to get the according usernames (for the user list). The number of users is determined using db_num_rows() on the result (to save an additional query, as in my revious patch), but this time the result is consistent with sess_count($time_period, false).

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

Ah, yeah my logged out user was still showing up. The JOIN fixes it. This was probably the root of the demonstrated problem in the first place.

Sorry for the confusion about sess_count, it is implemented correctly. It expects a timestamp for its first argument which is perfectly reasonable. $time_period in the block function is just a horrible name for the variable. At one point it was probably storing the time period from variable_get but now it is storing a timestamp derived from that period. The old authenticated user query was never changed to reflected this and that was the problem.

Tested with second user logging in and then logging out. Users and counts match and display correctly.

profix898’s picture

profix898 CreditAttribution: profix898 commented
FileSize
online_block_2.patch1.35 KB

Same patch as above, but with time_period renamed to timestamp. Choose what you like better.

Dries’s picture

Dries CreditAttribution: Dries commented

Actually, elsewhere in the code we write $interval. ;-)

profix898’s picture

profix898 CreditAttribution: profix898 commented
FileSize
online_block_3.patch1.35 KB

Whatever you like ... but in sess_count() it is called$timestamp already and even the db field is named 'timestamp'.
Again same patch as above, but with variable renamed to $interval :)

Wolfflow’s picture

Wolfflow CreditAttribution: Wolfflow commented

Online_block_3.patch applyed and result OK.
but I noticed a longer Frontpage Recall-time !!
Reference Site: http://www.adaccs.at/test

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Needs work

Has anyone looked at the performance implications? My brief review shows a filesort was added.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs work » Needs review
profix898’s picture

profix898 CreditAttribution: profix898 commented

Its a little difficult to benchmark this, because performance depends heavily on the number of users and sessions in the tables, but actually my install is even slightly faster with the patch.

I took a clean install of Drupal 5.0b1, created 500 users and 50 sessions, enabled the block and set FF2 to refresh every 5s (with all caching disabled incl. query cache, of course). After 250 refreshes I took a look a the query log ...

before (without patch):
sess_count() (500x): 7.41 ms (executed 2x per refresh, anonymous + registered users)
user_block() (250x): 9.81 ms
=> 2 x 7.41 ms + 9.81 ms = 24.63 ms

after (with patch):
sess_count() (250x): 7.43 ms (executed only 1x per refresh, anonymous users)
user_block() (250x): 12.98 ms
=> 7.43 ms + 12.98 ms = 20.41 ms

Although user_block() is slower with the JOIN query, the block is still faster than before (with the 2x sess_count()). I dont know how significant these results are!? Maybe someone can verify?

@drumm: What do you mean 'filesort was added'? If you are talking about the 'ORDER BY', it was already there.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Yeah, the filesort and temporary table are showing up because of the ORDER BY. I found the query doesn't use the temporary table when I used ORDER BY s.timestamp DESC. I think this provides the same functionality and should be a more effecient query.

profix898’s picture

profix898 CreditAttribution: profix898 commented
FileSize
online_block_4.patch1.35 KB

Right, using s.timestamp instead of u.access is better. All query parameters (WHERE, ORDER BY) are against the 'sessions' table then. Patch updated accordingly. I will repeat the benchmark with the new query as time permits.

profix898’s picture

profix898 CreditAttribution: profix898 commented
FileSize
online_block_5.patch1.43 KB

The query with s.timestamp is about 2x faster (12 ms => 6 ms). Resulting in a total query time of 13.87 ms (normalized to values reported in #13).

(New patch only removes a redundant blank line from patch in #15)

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

So we fix the bad behavior and have less time used by queries. Sounds like a win/win.

Patch works for me.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
fazal_rkmp’s picture

fazal_rkmp CreditAttribution: fazal_rkmp commented
FileSize
inconsistent_whos online_message.png4.51 KB

I am new to PHP and drupal. Can you tell me how to apply this patch.
As i am also facing lot of problem in my site. this this morning my site was running properly. but all of a sudden in the evening time i notice that, in Who's online block the number of user are 4000+.

Kindly mail me the step by step solution to fazal.dsr@gmail.com

Thanks in advance.