Re: Races in sessions

[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.


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  
> 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  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 
>>> 

Add a stable and documented setting to add files to the runserver watcher

[Diptesh Choudhuri]

I am working on a graphQL project which uses django and ariadne 
. This package supports 

 
loading your graphQL schema from external **.graphql *files. While 
developing the schema, I continually change these files and have to restart 
my *runserver *every time I do so, since the changes are not watched by 
default. This obviously gets quickly tiring since designing a GraphQL 
schema takes a lot of iterations.

There is a somewhat hacky fix 
 to this (here's 
 an 
implementation that I used in a project), but the problem with it is that 
it does not work at all for extensions like *django_extension's runserver 
*command. 
Additionally, it requires you to have to start an app, since it is defined 
in the *AppConfig, *something which might be inconvenient to some people. 

I propose we add a *AUTORELOADER_ADD_FILES *variable to the *settings.py *file 
which should be a Linux glob (eg: *AUTORELOADER_ADD_FILES = **BASE_DIR / 
"**/*.graphql"*).

If approved, I would like to make a PR for the same.

Regards
Diptesh Choudhuri

-- 
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/0164ea90-33a2-4ee5-b6cd-df508b6ab013n%40googlegroups.com.

Re: Races in sessions

['Adam Johnson' via Django developers (Contributions to Django itself)]

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 
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  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-developers+unsubscr...@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
>> 
>> .
>>
> --
> 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/CAD4ANxXYM6BPXLE0oq30w78BtxkV-ynoKBbObKyP7JORHO%2BJpQ%40mail.gmail.com
> 
> .
>


-- 
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/CAMyDDM2c7ZYU05qBKPGcms5vWj%3D0QXNV-_amdc1QvQL_%3DThMdQ%40mail.gmail.com.

Re: Races in sessions

[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).

On Sun, Feb 7, 2021 at 12:08 PM Matt  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-developers+unsubscr...@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
> 
> .
>

-- 
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/CAD4ANxXYM6BPXLE0oq30w78BtxkV-ynoKBbObKyP7JORHO%2BJpQ%40mail.gmail.com.

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?

-- 
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/89941f4b-61a2-446e-ba72-dfe428e0a9dbn%40googlegroups.com.