Opened 5 years ago

Last modified 8 days ago

#30448 new Bug

close_if_unusable_or_obsolete should skip connections in atomic block for autocommit check

Reported by: Daniel Hahler Owned by:
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords:
Cc: Jon Janzen Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes 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 (5)

comment:1 by Simon Charette, 5 years ago

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 by Daniel Hahler, 5 years ago

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 5 years ago by Daniel Hahler (previous) (diff)

comment:3 by Jacob Walls, 4 years ago

Has patch: set
Needs tests: set
Owner: changed from nobody to Daniel Hahler
Status: newassigned

comment:4 by Mariusz Felisiak, 3 months ago

Owner: Daniel Hahler removed
Status: assignednew

comment:5 by Jon Janzen, 8 days ago

Cc: Jon Janzen added
Note: See TracTickets for help on using tickets.
Back to Top