Opened 6 years ago

Last modified 4 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:
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords:
Cc: Jon Janzen, Evstifeev Roman, Anders Kaseorg 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 (8)

comment:1 by Simon Charette, 6 years ago

Triage Stage: Unreviewed → Accepted

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: new → assigned

​PR

comment:4 by Mariusz Felisiak, 8 months ago

Owner: Daniel Hahler removed
Status: assigned → new

comment:5 by Jon Janzen, 5 months ago

Cc: Jon Janzen added

comment:6 by Evstifeev Roman, 4 months ago

Cc: Evstifeev Roman added

comment:7 by Anders Kaseorg, 4 months ago

Cc: Anders Kaseorg added

Created ​https://github.com/django/django/pull/18393.

comment:8 by Carlton Gibson, 4 months ago

#35618 was a duplicate.

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