Allow transaction.atomic to work in async contexts.

[alex]

Wouldn't it be possible to add something like:

    __aenter__ = sync_to_async(Atomic.__enter__, thread_sensitive=True)
    __aexit__ = sync_to_async(Atomic.__exit__, thread_sensitive=True)

to the Atomic class to support async calls?

[Carlton Gibson]

Hi Alex.

Can I ask you to take the time to explain your issue report in full please? (Likely it's clear to you, but it needs more detail, perhaps starting from the beginning…)

Is it a duplicate of #32409?

Wouldn't it be possible to add something like...

Did you try this, perhaps with some test cases to exercise the new code?

[alex]

ok sorry was in a hurry. The code isn't tested yet so I cannot assure it is working.
Details follow

[alex]

django supports db transactions with rollback via the
django.db.transaction.atomic function which can either be used as decorator or as contextmanager.
Everything wrapped within is executed as transaction, see:

​https://docs.djangoproject.com/en/4.0/topics/db/transactions/

That is very nice but now there is a little problem:
it isn't async safe:

​https://forum.djangoproject.com/t/is-it-possible-to-use-transaction-atomic-with-async-functions/8924

When looking through the contextmanager (which is actually a class named Atomic in the same file) I saw that __enter__ and __exit__ use simple db operations for starting and commiting the transaction.
If we use the wrappers like suggested (__aenter__ and __aexit__) then everything should be fine in theory. In praxis I need to test the code (I have not much time so it may be easier if somebody else tries this out)

I build a wrapper class like this (of course it would be better to have it in Atomic itself so the atomic function can be used and no manual initialization is required):

from asgiref.sync import sync_to_async
from django.db.transaction import Atomic

class AsyncAtomic(Atomic):
    __aenter__ = sync_to_async(Atomic.__enter__, thread_sensitive=True)
    __aexit__ = sync_to_async(Atomic.__exit__, thread_sensitive=True)

Warning: untested

I must confess: the use case is very rare and is only useful in combination with select_for_update for delaying row updates (otherwise it is an antipattern as it causes slow transactions). And this is why I haven't a good example code.

[Carlton Gibson]

OK, thanks for the update Alex. I'm going to Accept this, since it's a desirable feature that transactions work in an async-context.

I'm half-inclined towards closing as needsinfo or marking as Someday/Maybe as I suspect the implementation will require a bit more than just adding the sync_to_async() wrappers. 🤔

I guess the first step would be to write some test cases for that and see what issues arise and what the performance looks like. (From there it's easier to see what's really involved.)

[HMaker]

I opened an issue at the channels repo ​https://github.com/django/channels/issues/1937 related to this.

The use case is not rare, it's pretty common on an async codebase. As you know, async code requires new API definitions on new namespaces, sync (blocking) and async code can't be mixed. Actually Django forces you to run the full transaction block inside @sync_to_async decorated functions, the async API becomes useless, you can't reuse any async code.

[rajdesai24]

Hey guys, I wanted to get your suggestion on this as I had been working on this and testing it out

class AsyncAtomic:
    def __init__(self, using=None, savepoint=True, durable=True):
        self.using = using
        self.savepoint = savepoint
        self.durable = durable

    async def __aenter__(self):
        self.atomic = transaction.Atomic(self.using, self.savepoint, self.durable)
        await sync_to_async(self.atomic.__enter__)()
        return self.atomic

    async def __aexit__(self, exc_type, exc_value, traceback):
        await sync_to_async(self.atomic.__exit__)(exc_type, exc_value, traceback)

---

class AsyncAtomicTestCase(TestCase):
    async def test_atomic(self):
        async with AsyncAtomic():
            # Create a new object within the transaction
            await sync_to_async(SimpleModel.objects.create)(
            field=4,
            created=datetime(2022, 1, 1, 0, 0, 0),
            )
        # Verify that the object was created within the transaction
        count = await sync_to_async(SimpleModel.objects.count)()
        self.assertEqual(count, 1)

[rajdesai24]

I made a new class called Async Atomic and used it as a context decorator, which worked but you still need to use the sync_to_async

[Mike Lissner]

I'm quite bad at async things generally, but I thought I'd chime in to say that I'm surprised async atomic transactions aren't more of a priority. A few of the comments above seem to imply that this isn't an important feature or that it's an antipattern (maybe?).

I just turned down part of a PR where a developer is converting our code to async because to do so required that we drop the @transaction.atomic decorator. I said, "Sorry, we can't covert this to async because given the choice between correctness and performance, I have to choose correctness."

Am I missing something big — Isn't this a big gap in Django's support for real applications converting fully to async?

Thanks all, sorry I don't have more to add! If I were better at async, I'd take a crack at actually fixing it.