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.
On Mon, 8 Feb 2021 at 02:58, Matt <m...@satchamo.com> wrote:
> 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.
>
>
> On Sunday, February 7, 2021 at 11:13:51 AM UTC-8 Adam Johnson wrote:
>
>> 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.
>>
>> On Sun, 7 Feb 2021 at 18:58, Stephen J. Butler <stephen...@gmail.com>
>> wrote:
>>
>>> 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).
>>>
>>> On Sun, Feb 7, 2021 at 12:08 PM Matt <ma...@satchamo.com> wrote:
>>>
>>>> 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?
>>>>
>>>> --
>>>> 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
>>>> <https://groups.google.com/d/msgid/django-developers/89941f4b-61a2-446e-ba72-dfe428e0a9dbn%40googlegroups.com?utm_medium=email&utm_source=footer>
>>>> .
>>>>
>>> --
>>> 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/CAD4ANxXYM6BPXLE0oq30w78BtxkV-ynoKBbObKyP7JORHO%2BJpQ%40mail.gmail.com
>>> <https://groups.google.com/d/msgid/django-developers/CAD4ANxXYM6BPXLE0oq30w78BtxkV-ynoKBbObKyP7JORHO%2BJpQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
>>> .
>>>
>>
>>
>> --
>> Adam
>>
> --
> 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-developers+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/django-developers/bd1850f9-9a46-4a2c-8694-63d1d8b11b02n%40googlegroups.com
> <https://groups.google.com/d/msgid/django-developers/bd1850f9-9a46-4a2c-8694-63d1d8b11b02n%40googlegroups.com?utm_medium=email&utm_source=footer>
> .
>
--
Adam
--
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-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/CAMyDDM2H8wTBVU1Fed_AJ_619o%3DKvQPqtB4JJRACO0F1Rbhn6A%40mail.gmail.com.
