Opened 7 years ago

Last modified 3 weeks ago

#30448 assigned Bug

close_if_unusable_or_obsolete should skip connections in atomic block for autocommit check

Reported by: Daniel Hahler Owned by: Samriddha Kumar Tripathi
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: no Patch needs improvement: yes
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 (11)

comment:1 by Simon Charette, 7 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, 7 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 7 years ago by Daniel Hahler (previous) (diff)

comment:3 by Jacob Walls, 6 years ago

Has patch: set
Needs tests: set
Owner: changed from nobody to Daniel Hahler
Status: new → assigned

​PR

comment:4 by Mariusz Felisiak, 2 years ago

Owner: Daniel Hahler removed
Status: assigned → new

comment:5 by Jon Janzen, 2 years ago

Cc: Jon Janzen added

comment:6 by Evstifeev Roman, 23 months ago

Cc: Evstifeev Roman added

comment:7 by Anders Kaseorg, 22 months ago

Cc: Anders Kaseorg added

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

comment:8 by Carlton Gibson, 22 months ago

#35618 was a duplicate.

comment:9 by Samriddha Kumar Tripathi, 3 months ago

Owner: set to Samriddha Kumar Tripathi
Status: new → assigned

comment:10 by Samriddha Kumar Tripathi, 3 months ago

Needs tests: unset

PR - ​https://github.com/django/django/pull/20836

Last edited 3 months ago by Samriddha Kumar Tripathi (previous) (diff)

comment:11 by Samriddha Kumar Tripathi, 3 weeks ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top