Opened 2 years ago
Last modified 5 weeks ago
#33882 assigned New feature
Allow transaction.atomic to work in async contexts.
Reported by: | alex | Owned by: | rajdesai24 |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.0 |
Severity: | Normal | Keywords: | async |
Cc: | Hugo Osvaldo Barrera, rajdesai24, Moritz Ulmer, Tyson Clugg, Julien Enselme | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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?
Change History (21)
comment:2 by , 2 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:3 by , 2 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
ok sorry was in a hurry. The code isn't tested yet so I cannot assure it is working.
Details follow
comment:4 by , 2 years ago
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:
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.
comment:5 by , 2 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Keywords: | async added |
Triage Stage: | Unreviewed → Accepted |
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.)
comment:6 by , 2 years ago
Summary: | async transaction.atomic → Allow transaction.atomic to work in async contexts. |
---|
comment:7 by , 2 years ago
Description: | modified (diff) |
---|
comment:8 by , 2 years ago
Cc: | added |
---|
comment:9 by , 2 years ago
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.
comment:10 by , 21 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 21 months ago
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)
comment:12 by , 21 months ago
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
comment:13 by , 21 months ago
Cc: | added |
---|
comment:14 by , 14 months ago
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.
comment:15 by , 14 months ago
Cc: | added |
---|
comment:16 by , 12 months ago
Cc: | added |
---|
comment:17 by , 11 months ago
Cc: | added |
---|
comment:18 by , 2 months ago
I am hesitant going for async django until transaction support is added or a cleaner solution than having to use sync_to_async is available. The problem with this workaround is that it forces you to write all code inside the transaction as sync. Which is going to be a mess when transaction and non-transaction scopes want to use the same code/logic.
The following example illustrate the problem
async def post(): await sync_to_async(_post_sync_transaction) # <-- Messy, but maybe acceptable @transaction.atomic def _post_sync_transaction(): logic1() async_to_sync(alogic2) # <-- Very messy, not acceptable async_to_sync(alogic3) # <-- Very messy, not acceptable async def get(): await alogic2() await alogic3() def logic1(): # ... async def alogic2(): #... async def alogic3(): # ...
There are two problems with this pattern:
- You have to write helper functions to handle transactions
- You have to make sure to convert all async contexes to sync, using async_to_sync, within the transaction helper
follow-up: 20 comment:19 by , 6 weeks ago
I agree. Why hasn't it been fixed yet? Multiple years have passed already.
comment:20 by , 6 weeks ago
Replying to Denis Orehovsky:
I agree. Why hasn't it been fixed yet? Multiple years have passed already.
Hello Denis! Please note that this is not a helpful comment, I understand that you may feel frustrated but I encourage you to consider our Code of Conduct in your comments:
Be friendly and patient.
Be welcoming.
Be considerate.
Be respectful.
Be careful in the words that you choose.
Django is developed by volunteers who contribute their time when they can. If you're interested in this feature, it would be fantastic if you could help by contributing to a solution.
Thank you for your understanding!
comment:21 by , 5 weeks ago
I can take this one, but how would I test it after implementation? (manual testing I mean)
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?