Throttle set_session_expires. #2041
Conversation
| return | ||
| c.run( "UPDATE participants SET session_expires=%s " | ||
| "WHERE id=%s AND is_suspicious IS NOT true" | ||
| , (expires, self.id,) |
There was a problem hiding this comment.
I've never quite understood with self.db.get_cursor() as c:. I just looked up what with does, and that's actually pretty cool. So, going further:
- What does
self.db.get_cursor()do to this code? What would happen without it? Does theFOR UPDATElock expire at the end of thewithblock? - I'm confused how this fixes the problem of multiple threads trying to update the same data. It seems like now those threads might each still attempt the SQL
UPDATEat the same time. It's just that now all but one would fail, because the first thread locked the row.
There was a problem hiding this comment.
The lock expires with the end of the transaction. When a thread encounters a locked row, it blocks until the lock is released. Therefore only the first thread does the update, the next ones get the updated value and skip the update. Now thinking about it, it can be made simpler.
Please do not merge for now.
|
@seanlinsley How about now? |
| if done is None: | ||
| return | ||
| add_event(c, 'participant', | ||
| dict(action='set', id=self.id, values=dict(session_expires=unicode(expires)))) |
There was a problem hiding this comment.
Is there really a need to record this event? Seems like that's going to bloat the database without adding value.
There was a problem hiding this comment.
The final goal here is to be able to recreate the whole db the events table. So all changes should be logged here. See #1549 (comment) for the general plan.
There was a problem hiding this comment.
Why would we ever want to recreate session_expires? Why are we storing this in the database in the first place?
There was a problem hiding this comment.
It seems like a signed / encrypted cookie would be a much better approach.
There was a problem hiding this comment.
💡 If only you had asked me 17 days ago 😀. I have been trying to find with 🔦 where it is used for some time now. It seems.... surprise surprise... that nowhere
Just dumping the column session_expires and all code related to it would not change a bit about the site behavior. The participant object is just created from session_token gittip/security/authentication.py#L49. Grepping for expires brings up only code related to writing it.
There was a problem hiding this comment.
35% of our participants table has an expired session (session_token is not null and session_expires < now()).
There was a problem hiding this comment.
(P.S. 1.3% have a non-expired session.)
There was a problem hiding this comment.
I just killed old sessions with this:
update participants set session_token=null where session_expires < now();
Looks like we don't reset session_expires in Participant.end_session, which is also a bug. But we should probably clean up such things in a separate PR, eh?
There was a problem hiding this comment.
@zwn @seanlinsley So what's the next step here?
There was a problem hiding this comment.
- update this PR so we don't store an event
- update the code so we don't respect expired sessions
- (optional, after this PR) instead of storing in the database, sign the cookie with something like tokenlib, ref: move caching out of memory #715
|
In an effort to get through our outstanding PRs, I'm going to take this on. |
|
@seanlinsley Thanks. The original point of this PR was to "fix" writing to |
|
@seanlinsley How can I help get this one done? Do we need a hackaton? |
|
Just found #2041 (comment) thanks to @seanlinsley . I'll check it out tonight. |
set_session_expires used to update the field `session_expires` in the participants table on each http hit.
When we do hit the database, it is going to be hit from several threads at the same time. Thanks to the WHERE condition only one of them wins.
|
Closing this in favor of #2332. Deleting the branch via the Github UI so we can restore it later if needed. |
set_session_expires used to update the field
session_expiresin the participantstable on each http hit.
From #1878 (comment)