Posted by profix898 on November 5, 2006 at 11:08am
| Project: | Drupal core |
| Version: | 5.0-beta1 |
| Component: | user.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| online_block.png | 3.81 KB | Ignored: Check issue status. | None | None |
Comments
#1
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.#2
This patch also corrects the
time_period, currently users are never removed from the list. The query checks foraccess time >= period(period = 900 by default) instead ofaccess time >= time() - period.#3
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.
#4
But isn't the implementation of sess_count() wrong than? (We use that function in a couple of places.)
#5
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 usingdb_num_rows()on the result (to save an additional query, as in my revious patch), but this time the result is consistent withsess_count($time_period, false).#6
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.
#7
Same patch as above, but with
time_periodrenamed totimestamp. Choose what you like better.#8
Actually, elsewhere in the code we write $interval. ;-)
#9
Whatever you like ... but in
sess_count()it is called$timestampalready and even the db field is named 'timestamp'.Again same patch as above, but with variable renamed to
$interval:)#10
Online_block_3.patch applyed and result OK.
but I noticed a longer Frontpage Recall-time !!
Reference Site: http://www.adaccs.at/test
#11
Has anyone looked at the performance implications? My brief review shows a filesort was added.
#12
#13
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 2xsess_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.
#14
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.#15
Right, using
s.timestampinstead ofu.accessis 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.#16
The query with
s.timestampis 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)
#17
So we fix the bad behavior and have less time used by queries. Sounds like a win/win.
Patch works for me.
#18
Committed to CVS HEAD. Thanks.
#19