Race condition in session save.

[tevans]

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.

[Aymeric Augustin]

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 known session_key).

Indeed, create() calls save(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.

---

(...) so you cannot reliably save() an unsaved session object.

The obvious answer here is to use create() rather than save() 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() and create(). Currently save() 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:

• this is backwards incompatible,
• the docs contain an example which initiates a new session and saves it,
• the probability of an collision is extremely low.

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.

[Aymeric Augustin]

#16484 probably reports the same race condition.

[Anssi Kääriäinen]

I wonder how you will hit this race condition? Wouldn't getting the same session_key be extremely random? Am I missing something here?

[Aymeric Augustin]

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?

[Aymeric Augustin]

This comment may also be relevant.