Opened 14 months ago

Last modified 14 months ago

#30601 new Cleanup/optimization

Extend documentation about transaction rollbacks to mention global state mutations such as caching

Reported by: Sebastian Wagner Owned by: nobody
Component: Documentation Version: 2.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

ATOMIC_REQUESTS=True essentially creates nested transactions. And nested transactions might not play well with custom caching.

A small example of how to use nested transactions and caching to get inconsistent data:

my_mem_cache_a = ...
my_mem_cache_b = ...

def set_a(x):
    with transaction.atomic():
        a = ...
        a.x = x
        a.save()
    my_mem_cache_a = x 

def set_a_b(x, y):
    with transaction.atomic():
        set_a(x)
        b = ...
        b.y = y
        raise Exception()
        # at this point, my_mem_cache_a is out of sync with the database. 
        b.save()
    my_mem_cache_b = x 

set_a_b(...)

(this example also works without transaction.atomic() )

Would be nice to add a warning to

https://docs.djangoproject.com/en/2.2/topics/db/transactions/#tying-transactions-to-http-requests

that ATOMIC_REQUESTS=True might be harmful, if code

  • uses caching
  • proactively updates data in the cache
  • makes the assumption that values are updated in the database after a transaction is finished.

Change History (1)

comment:1 Changed 14 months ago by Simon Charette

Component: Database layer (models, ORM)Documentation
Needs documentation: set
Summary: ATOMIC_REQUESTS=True and caching might be dangerousExtend documentation about transaction rollbacks to mention global state mutations such as caching
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

I'm not sure how related to ATOMIC_REQUEST this issue actually is, it seems like a common pitfall of mixing database transactions and caching and a good reason to rely on transaction.on_commit to defer cache alterations instead.

The atomic documentation already mentions that model state should be reverted (#28479) so maybe we could adjust this block to mention that all form of state, including global one such as caching, should be reverted and that using transaction.on_commit can be used to deal with global state alterations.

Tentatively accepting based on the premise you might be interested in submitting a patch yourself.

Note: See TracTickets for help on using tickets.
Back to Top