Opened 8 years ago
Last modified 3 years ago
#27074 new Cleanup/optimization
connection.is_usable() raises AttributeError after the connection is closed
Reported by: | Chris Jerdonek | Owned by: | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.10 |
Severity: | Normal | Keywords: | |
Cc: | boblefrag@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description ¶
I know a similar issue was closed relatively recently as won't fix, but I wanted to report a more obvious example of the same issue just to be sure.
The following test case--
from django.db import connection from django.test import TransactionTestCase class MyTest(TransactionTestCase): def test_connection_close(self): self.assertTrue(connection.is_usable()) connection.close() self.assertFalse(connection.is_usable())
results in the following error (though if TransactionTestCase
is replaced by TestCase
, it would behave as expected)--
Traceback (most recent call last): ... File ".../test_database.py", line 23, in test_connection_close self.assertTrue(connection.is_usable()) File ".../site-packages/django/db/backends/postgresql/base.py", line 229, in is_usable self.connection.cursor().execute("SELECT 1") AttributeError: 'NoneType' object has no attribute 'cursor'
Would there be any harm in handling the case of self.connection
being None
(e.g. using EAFP)? It seems like this could even be done so the logic is only in the base class.
According to the ticket's flags, the next step(s) to move this issue forward are:
- To provide a patch by sending a pull request. Claim the ticket when you start working so that someone else doesn't duplicate effort. Before sending a pull request, review your work against the patch review checklist. Check the "Has patch" flag on the ticket after sending a pull request and include a link to the pull request in the ticket comment when making that update. The usual format is:
[https://github.com/django/django/pull/#### PR]
.
Change History (13)
comment:1 by , 8 years ago
Summary: | connection.is_usable() can raise AttributeError in natural uses → connection.is_usable() raises AttributeError after the connection is closed |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 8 years ago
Cc: | added |
---|
comment:3 by , 8 years ago
Unless I missed something, this code matches the trackback reported above, so the issue likely still exists.
The fix might be as simple as returning False if self.connection is None
(assuming that doesn't break tests).
comment:5 by , 8 years ago
I believe the trickiest part of the patch will be handling / dealing with the fact that there are multiple backend implementations of is_usable()
.
Ideally, I think one would want to put the logic in the base implementation and call super()
as appropriate. Alternatively, it looks like it would be sufficient to include the logic in all three non-trivial implementations: mysql, oracle, and postgresql. Those each look like the following:
def is_usable(self): try: self.connection.ping() except Database.Error: return False else: return True
comment:7 by , 8 years ago
Also, rather than checking for connection is None
in advance, using EAFP and checking for None
in an except
block (e.g. handling AttributeError) is probably more Pythonic.
comment:8 by , 8 years ago
For future reference, an argument for putting the logic in the base implementation is that it's the base implementation that sets self.connection
to None
in the first place (in the finally block of its close()
method, when not in an atomic block). See here for the code.
comment:9 by , 8 years ago
This is a private API that Django primarily calls inside an if self.connection is not None:
block (and secondarily in testing tools where database connection breakage isn't a concern and will abort tests in general). For this reason, I don't expect this issue to appear through the use of public APIs.
I'm -0 on making that change but I understand that we might have to add a useless check just to stop these bugs reports. The performance impact will be negligible.
comment:10 by , 8 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:11 by , 8 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:12 by , 8 years ago
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
I will now put together a PR for this, which is what I was originally planning to do when I first created this ticket (but someone else opened one before me).
comment:13 by , 3 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
This is no longer the case on django master branch: