Races in sessions

[Matt]

The "post_comment" example of sessions appears to be incorrect: https://docs.djangoproject.com/en/3.1/topics/http/sessions/#examples

Imagine two HTTP requests coming in at the same time, both seeing that "has_commented" is False, and then both create a comment and set the session variable to True.

I just experienced this issue myself, which is why I bring it up.

I would offer to update the documentation...but I'm struggling to come up with a way to actually make this pattern work with sessions! Assuming there are other views that update other variables in the session, it seems like you must lock the session when you read it (i.e. "SELECT...FOR UPDATE"). So anything that reads or writes to the session must be wrapped in an atomic block.

I think it might be useful to add a method to support locking the session before any reads/writes. Or maybe we should just update the docs to warn people about how race prone sessions are?

Any thoughts?

[Stephen J. Butler]

The way I'd solve this example is to create a unique constraint/index in the DB. The session check gives a quick pre-check, but in the event of a race the DB enforces absolute consistency.

If the constraint isn't possible or is too complex, then your "SELECT... FOR UPDATE" belongs on the check for whether someone is allowed to comment, not on the session. Select for update is a kind of locking, and the rule for locking is to do it as little as possible, in as isolated section of code as possible. So you do it around the code that checks if a user can create a comment and then creates it (which will probably be a small percent of your requests) vs. doing it in the session logic which gets run for 100% of requests (but is unneeded 99.99% of the time).

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/89941f4b-61a2-446e-ba72-dfe428e0a9dbn%40googlegroups.com.

[Adam Johnson]

Stephen - you're right that a constraint is the best way to enforce consistency in the DB. That doesn't fit every use case for sessions though - people use Django's built-ins with many different kinds of data stores that don't support locking or constraint semantics, such as remote API's.

Matt - I think we could remove that example.

As for a new session locking method, I'm not sure how feasible it is *in general* since the session API is designed to back onto any kind of data store. If you had an example implementation it would be good to see and know how well it performs in a real world setting.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAD4ANxXYM6BPXLE0oq30w78BtxkV-ynoKBbObKyP7JORHO%2BJpQ%40mail.gmail.com.

--

Adam

[Matt]

Thinking out loud: You can't just lock the session when you think you have a critical section of code in a view because the session data is stored in one field in the database, and is completely overwritten on write. Consider this case:

Thread A: Read session data
Thread B: Open transaction, read session (with lock) data, add session['foo'] = 1, write session back to database, close transaction.
Thread A: session['bar'] = 1, save session (completely overwriting Thread B's changes so 'foo' doesn't exist anymore)

So any view that will write to the session *must* lock it before reading. And like Stephen is saying, you don't want to do that on every request. I suppose you could keep session reads as-is. But any time you need to do a write, you must explicitly wrap it in a transaction, and refresh the data from the database (while locking other writers):

...
# freely read any session data (but it could be stale)
...
# now we need to write something to the session
with transaction.atomic():
    # the session data needs to be refresh if it was updated by another transaction after it was first read by this thread
    request.session.lock_and_refresh()
    if request.session.get('has_commented', False):
        request.session['has_commented'] = True
        ...
    request.session.save()

Session implementations could provide a context manager (if they support atomic writes) so it's not so ugly:

with request.session.atomic():
    if request.session.get("has_commented", False):
        request.session['has_commented'] = True
        ...

But EVERY session write must follow that pattern, or it's all for naught. I don't think Django core uses sessions much, so updating core doesn't sound bad. It's everyone downstream. That seems totally impractical. I do think there are some very subtle bugs that would be resolved by adopting something like this. But it may not be worth the trouble.

[Adam Johnson]

Indeed, locking is hard. I think it'd be better to warn more about how sessions do not have guarantees against parallel writes. And perhaps we could update the example to use the session as a cheap first check whether the user has commented, actually defending with a full query against the database.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/bd1850f9-9a46-4a2c-8694-63d1d8b11b02n%40googlegroups.com.

--

Adam