#18344 closed Bug (needsinfo)
Race condition in session save.
Reported by: | tevans | Owned by: | nobody |
---|---|---|---|
Component: | contrib.sessions | Version: | 1.4 |
Severity: | Normal | Keywords: | session_key |
Cc: | Triage Stage: | Design decision needed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Using the DB backend, SessionStore.save() follows this logic:
Generate a session key by calling self._get_or_create_session_key()
_get_or_create_session_key() generates a random key, and then tests to see if it exists in the database.
Once it finds one which does not exist in the database, it returns the key.
save() then tries one time to persist the session into the database.
If this raises an IntegrityError, due to the same key being allocated to another client simultaneously, then the error is raised, and no further processing takes place.
The SessionStore.create() method avoids this situation by repeatedly attempting to persist a session, changing the session key and trying again if this fails.
This is not possible to do from outside the session class (well, you can, you have to use _non_public_apis() to do it), and so you cannot reliably save() an unsaved session object.
Change History (5)
comment:1 by , 12 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 12 years ago
I wonder how you will hit this race condition? Wouldn't getting the same session_key be extremely random? Am I missing something here?
comment:4 by , 12 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
I agree that it's very unlikely to trigger this race condition by chance, given the length of session keys.
This ticket may be based on code analysis and not on an actual error (it doesn't contain a traceback).
If this error happens in production, could you provide a traceback?
As far as I can tell:
create()
is designed to create a new session (ie. not loaded from the session store,session_key = None
)save()
is designed to save an existing session (ie. loaded from the session store with a knownsession_key
).Indeed,
create()
callssave(must_create=True)
in a loop until an unused session key is found. That's an implementation detail and I'm just mentioning it for clarity.The obvious answer here is to use
create()
rather thansave()
to create a new session. That's the way to reliably save an unsaved/new session.Once the session is saved its
session_key
is defined and won't change.That said, calling
save()
on a new session will also work: as explained by Tom,save()
will create a new session key if none exists. The new key may collide with an existing one (with an extremely low probability).Considering this problem, here are the actions we could take:
1) Improve the documentation to clarify what
save()
andcreate()
. Currentlysave()
is mentioned in the docs but its behavior isn't described in detail.create()
isn't mentioned at all. This is probably a good idea and it's tracked in #18348.2) Change
save()
to raise an exception if no session key exists yet. But beware:3) Introduce a new function that performs a "save or create". A pre-requisite would be to formalize a public API for session stores, and prove that such a function is useful — after all it's just
session.create() if session.session_key is None else session.save()
. This is a much larger endeavor than fixing a race condition. It'd require a solid proposal on django-developers.That's my brain dump on this issue. This ticket is really about deciding what we do for point (2) above.
I won't decide now; I'm sufficiently chilled by the tone of our recent discussion on django-developers.