[Django] #34369: Improve the interaction between transaction.atomic() and generator functions

[Django]

#34369: Improve the interaction between transaction.atomic() and generator
functions
-------------------------------------+-------------------------------------
Reporter: Raphaël | Owner: nobody
Barrois |
Type: | Status: new
Uncategorized |
Component: Database | Version: 4.1
layer (models, ORM) |
Severity: Normal | Keywords: atomic generator
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When wrapping a generator function with `@transaction.atomic()`, the
decorator doesn't apply during the generator's execution.

This is a standard behaviour of all decorators for all generators — when
the decorator calls the decorated function, that function returns
immediately with the generator object — and doesn't execute even a single
line of code.

Thus, in the following code example:

{{{#!python
@transaction.atomic()
def frobnicate_users(qs):
for user in qs.select_for_update():
yield user.spam_it()
}}}

The code will fail at runtime, since the `.select_for_update()` is called
**after** returning from the `@transaction.atomic()` block.

I believe it would make sense for Django to improve this interaction;
possible options would be:
- Having `transaction.atomic()` use `inspect.isgeneratorfunction(wrapped)`
to raise when it is decorating a generator function (with guidance to
replace it with a `with transaction.atomic()` in the function body);
- Having `transaction.atomic()` automatically adjust itself to generator
functions — if the wrapped function is a generator function, place the
atomic block around a new generator wrapping the returned generator;
- Have `transaction.atomic()` raise a warning if the value returned by its
wrapped function is a generator.

It might make sense to add an optional kwarg to `transaction.atomic()` to
control that behaviour — for instance, this improvement should not alter
the handling of `StreamingResponse`.

If there is a consensus on moving this forward, I will happily provide a
related pull-request.

--
Ticket URL: <https://code.djangoproject.com/ticket/34369>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#34369: Improve the interaction between transaction.atomic() and generator
functions
-------------------------------------+-------------------------------------

Reporter: Raphaël Barrois | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: atomic generator | Triage Stage:

| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* type: Uncategorized => Cleanup/optimization

Comment:

Thanks for the report, however I'm not sure it's worth additional
complexity. This is clearly a misuse of the decorator, I'm not convinced
that we need to document or tweak anything here.

--
Ticket URL: <https://code.djangoproject.com/ticket/34369#comment:1>

[Django]

#34369: Improve the interaction between transaction.atomic() and generator
functions
-------------------------------------+-------------------------------------
Reporter: Raphaël Barrois | Owner: nobody

Type: | Status: closed

Cleanup/optimization |
Component: Database layer | Version: 4.1
(models, ORM) |

Severity: Normal | Resolution: wontfix

Keywords: atomic generator | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Carlton Gibson):

* status: new => closed
* resolution: => wontfix

Comment:

I'm going to close based on Mariusz' comment. I tend to agree. The docs
that pretty clear that the transaction is closed when the block exits, so
this is expected behaviour. (Maybe it's just me but, in all these years, I
never thought to use a generator in this context.)

--
Ticket URL: <https://code.djangoproject.com/ticket/34369#comment:2>

[Django]

#34369: Improve the interaction between transaction.atomic() and generator
functions
-------------------------------------+-------------------------------------
Reporter: Raphaël Barrois | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: atomic generator | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Raphaël Barrois):

Fair enough; I guess this should instead be seen as an issue in
`contextlib`, which could detect when it's decorating generators and
adjust its behaviour accordingly.

--
Ticket URL: <https://code.djangoproject.com/ticket/34369#comment:3>