Opened 15 months ago

Last modified 11 months ago

#30448 new Bug

close_if_unusable_or_obsolete should skip connections in atomic block for autocommit check

Reported by: Daniel Hahler Owned by: nobody
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Via https://github.com/django/channels/issues/1091#issuecomment-489488831 I have noticed that close_if_unusable_or_obsolete will close the DB connection used in tests, which has entered an atomic block (via TestCase).

channel's DatabaseSyncToAsync calls close_if_unusable_or_obsolete here.

The following patch might make sense:

diff --git c/django/db/backends/base/base.py i/django/db/backends/base/base.py
index 9fa03cc0ee..f9ca0f8464 100644
--- c/django/db/backends/base/base.py
+++ i/django/db/backends/base/base.py
@@ -497,7 +497,10 @@ def close_if_unusable_or_obsolete(self):
         if self.connection is not None:
             # If the application didn't restore the original autocommit setting,
             # don't take chances, drop the connection.
-            if self.get_autocommit() != self.settings_dict['AUTOCOMMIT']:
+            if (
+                    not self.in_atomic_block and
+                    self.get_autocommit() != self.settings_dict["AUTOCOMMIT"]
+            ):
                 self.close()
                 return
 

Change History (2)

comment:1 Changed 15 months ago by Simon Charette

Triage Stage: UnreviewedAccepted

close_if_unusable_or_obsolete is certainly an internal API but I agree that not considering self.in_atomic_block in the check against settings_dict["AUTOCOMMIT"] is way too naive to determine the connection is unusable or obsolete.

comment:2 Changed 11 months ago by Daniel Hahler

Created https://github.com/django/django/pull/11769.

but I agree that not considering self.in_atomic_block in the check against settings_dict["AUTOCOMMIT"] is way too naive to determine the connection is unusable or obsolete.

Not sure I understand that correctly: do you think the patch makes sense, or is still to naive?

Last edited 11 months ago by Daniel Hahler (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top