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

[Adam Johnson]

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

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

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

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

[Aymeric Augustin]

Hello,

On 18 Nov 2021, at 11:11, Florian Apolloner <f.apo...@gmail.com> 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.

[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

[Carlton Gibson]

On Sat, 20 Nov 2021 at 14:21, Florian Apolloner <f.apo...@gmail.com> 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. :)