Re: select_for_update and transactions (bad dev UX in a feature)

[Carlton Gibson]

On Sat, 20 Nov 2021 at 14:21, Florian Apolloner 
wrote:

> I even have a possible fix for that, but it requires us to rewrite
> middlewares again :/
>

This is the third time this has come up (the middleware rewrite I mean)
Rule of Threes. We need a DjangoCon, so we can draft a DEP. :)

>

-- 
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/CAJwKpyTvkZ5NYLZq8H6kJRVGEtjpqs8u%2BnWqQV97ZG%2Bme8aqmg%40mail.gmail.com.

Re: select_for_update and transactions (bad dev UX in a feature)

[Florian Apolloner]

Hi Aymeric,

On Saturday, November 20, 2021 at 12:39:17 PM UTC+1 Aymeric Augustin wrote:

> I'm not trying to disagree for the sake of disagreement; I'm just trying 
> to bring some contextual awareness and avoid  the "core devs say 
> ATOMIC_REQUESTS is bad" effect. I hope we can agree on this?
>

Absolutely, my comment was mostly there to show that not all core devs 
agree that ATOMIC_REQUESTS is something that should be always enabled. As 
usual it depends on your project, database etc… I should have added more 
context to that, thanks for pointing it out.
   

> Finally — yes, I know that my implementation of this feature is a wormhole 
> across layers. I won't even try to justify it. I wish we had something 
> better but we haven't found that yet.
>

I still think it is good (in the sense that we worked with what we had 
available). Sure the layering isn't exactly nice, but we have worse things 
in Django's codebase :) I even have a possible fix for that, but it 
requires us to rewrite middlewares again :/  

Cheers,
Florian

-- 
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/6d65a33b-32ad-46a0-9b75-9c7f96c1fe03n%40googlegroups.com.

Re: select_for_update and transactions (bad dev UX in a feature)

[Aymeric Augustin]

Hello,

> On 18 Nov 2021, at 11:11, Florian Apolloner  wrote:
> 
> FWIWI I always recommend disabling ATOMIC_REQUESTS and using transactions  as 
> needed :)

Investing engineers' time into evaluating the exact transactional integrity 
requirements of every view may be appropriate in some contexts. Getting a 
blanket safeguard from ATOMIC_REQUESTS can be a fair tradeoff in other contexts.

Engineer time is expensive. PostgreSQL horsepower can be pretty cheap on low or 
even medium traffic websites, which are a substantial part of Django's target 
audience.

On these websites, it can be entirely reasonable to pay a limited cost for 
ATOMIC_REQUESTS and to get the benefit that every view succeeds completely or 
fails completely.

Also you don't depend on engineers getting the analysis right every time, 
including when they make a change — the situation that triggered this 
discussion in the first place.

> Honestly long transactions are imo bad, one should evaluate what the view 
> needs and then act accordingly.


Since most views on most websites are short, it isn't completely bonkers to use 
a blacklist approach, that is, to review the situation closely on long views 
and apply non_atomic_requests appropriately.

I'm not trying to disagree for the sake of disagreement; I'm just trying to 
bring some contextual awareness and avoid  the "core devs say ATOMIC_REQUESTS 
is bad" effect. I hope we can agree on this?

Finally — yes, I know that my implementation of this feature is a wormhole 
across layers. I won't even try to justify it. I wish we had something better 
but we haven't found that yet.

-- 
Aymeric.

-- 
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/16779272-37F9-4BB6-A5F7-4CE5B226E2A9%40polytechnique.org.

Re: select_for_update and transactions (bad dev UX in a feature)

[Florian Apolloner]

On Wednesday, November 17, 2021 at 11:09:58 PM UTC+1 Adam Johnson wrote:

> FWIW I always recommend enabling ATOMIC_REQUESTS and ensuring transactions 
> are used by default in other code paths such as background tasks.
>

FWIWI I always recommend disabling ATOMIC_REQUESTS and using transactions  
as needed :) Honestly long transactions are imo bad, one should evaluate 
what the view needs and then act accordingly.

-- 
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/ef5689c4-506a-4533-ab31-89ce496e2e43n%40googlegroups.com.

Re: select_for_update and transactions (bad dev UX in a feature)

[Klemen Štrajhar]

Hi Adam

I will check into it.

IMO using ATOMIC_REQUESTS is an antipattern. Views shouldn't know anything 
about the database. Services should handle all persistence related stuff 
and that's why transactions should only be used when necessary.

Best!
Klemen

sreda, 17. november 2021 ob 23:09:58 UTC+1 je oseba Adam Johnson napisala:

> Hi Klemen
>
> We recently changed atomic() to mark when it is created by the test runner 
> for this ticket: https://code.djangoproject.com/ticket/33161 . This was 
> for special case logic around the 'durable' flag to atomic().
>
> Perhaps the newly added tracking can also be used for the check in 
> select_for_update(), and perhaps other things that require an atomic() 
> (i.e. they check connection.in_atomic_block). You could investigate if a 
> patch is feasible.
>
> FWIW I always recommend enabling ATOMIC_REQUESTS and ensuring transactions 
> are used by default in other code paths such as background tasks.
>
> Thanks,
>
> Adam
>
> On Wed, 17 Nov 2021 at 21:02, Klemen Štrajhar  
> wrote:
>
>> Hi!
>> I noticed dangerous behaviour with testing and usage of *select_for_update 
>> *in transactions.
>> This "feature" crashed some parts of production in our company.
>> The "feature" I am talking about, is that test runner wraps all tests in 
>> a transaction. If you use *select_for_update* you have to wrap it in a 
>> transaction.atomic(). That is quite obvious as django raises an error if 
>> query is not in a transaction.
>> This is what happened at our company:
>>
>>1. One code block was wrapped in transaction atomic where 
>>select_for_update was used. The code was tested in a django test.
>>2. transaction.atomic context manager was accidentally removed. The 
>>test still passed as test runner wraps test in it's own transaction.
>>3. Code was happily deployed
>>4. On production server there was no more transaction wrapper and 
>>exception was raised at select_for_update
>>5. ...
>>6. Hotfix galore
>>
>> Is there any option to separate the transaction wrapper for testing and 
>> normal code execution? I am not familiar with internals of test runner 
>> etc., but would gladly take a look.
>> What are your opinions about this?
>> - Klemen
>>
>> -- 
>> 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/be5bd163-5915-4890-b225-78094c773eb8n%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/db53c546-65ec-4b2a-b1c3-9fac4e3f041cn%40googlegroups.com.

Re: select_for_update and transactions (bad dev UX in a feature)

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

Hi Klemen

We recently changed atomic() to mark when it is created by the test runner
for this ticket: https://code.djangoproject.com/ticket/33161 . This was for
special case logic around the 'durable' flag to atomic().

Perhaps the newly added tracking can also be used for the check in
select_for_update(), and perhaps other things that require an atomic()
(i.e. they check connection.in_atomic_block). You could investigate if a
patch is feasible.

FWIW I always recommend enabling ATOMIC_REQUESTS and ensuring transactions
are used by default in other code paths such as background tasks.

Thanks,

Adam

On Wed, 17 Nov 2021 at 21:02, Klemen Štrajhar 
wrote:

> Hi!
> I noticed dangerous behaviour with testing and usage of *select_for_update
> *in transactions.
> This "feature" crashed some parts of production in our company.
> The "feature" I am talking about, is that test runner wraps all tests in a
> transaction. If you use *select_for_update* you have to wrap it in a
> transaction.atomic(). That is quite obvious as django raises an error if
> query is not in a transaction.
> This is what happened at our company:
>
>1. One code block was wrapped in transaction atomic where
>select_for_update was used. The code was tested in a django test.
>2. transaction.atomic context manager was accidentally removed. The
>test still passed as test runner wraps test in it's own transaction.
>3. Code was happily deployed
>4. On production server there was no more transaction wrapper and
>exception was raised at select_for_update
>5. ...
>6. Hotfix galore
>
> Is there any option to separate the transaction wrapper for testing and
> normal code execution? I am not familiar with internals of test runner
> etc., but would gladly take a look.
> What are your opinions about this?
> - Klemen
>
> --
> 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/be5bd163-5915-4890-b225-78094c773eb8n%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/CAMyDDM31-kaB6CZVZ2Hk-0A62wAF7-95ifEKYnY%2BDXY4DfkmWw%40mail.gmail.com.

select_for_update and transactions (bad dev UX in a feature)

[Klemen Štrajhar]

Hi!
I noticed dangerous behaviour with testing and usage of *select_for_update *in 
transactions.
This "feature" crashed some parts of production in our company.
The "feature" I am talking about, is that test runner wraps all tests in a 
transaction. If you use *select_for_update* you have to wrap it in a 
transaction.atomic(). That is quite obvious as django raises an error if 
query is not in a transaction.
This is what happened at our company:

   1. One code block was wrapped in transaction atomic where 
   select_for_update was used. The code was tested in a django test.
   2. transaction.atomic context manager was accidentally removed. The test 
   still passed as test runner wraps test in it's own transaction.
   3. Code was happily deployed
   4. On production server there was no more transaction wrapper and 
   exception was raised at select_for_update
   5. ...
   6. Hotfix galore

Is there any option to separate the transaction wrapper for testing and 
normal code execution? I am not familiar with internals of test runner 
etc., but would gladly take a look.
What are your opinions about this?
- Klemen

-- 
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/be5bd163-5915-4890-b225-78094c773eb8n%40googlegroups.com.