Opened 8 years ago

Last modified 2 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.

Change History (13)

comment:1 by Tim Graham, 8 years ago

Summary: connection.is_usable() can raise AttributeError in natural usesconnection.is_usable() raises AttributeError after the connection is closed
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 by Yohann Gabory, 8 years ago

Cc: boblefrag@… added

This is no longer the case on django master branch:

    def is_usable(self):
        try:
            # Use a psycopg cursor directly, bypassing Django's utilities.
            self.connection.cursor().execute("SELECT 1")
        except Database.Error:
            return False
        else:
            return True

comment:3 by Aymeric Augustin, 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:4 by Yohann Gabory, 8 years ago

My bad, I was testing with sqlite3. I can reproduce with postgresql

comment:5 by Chris Jerdonek, 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 Chris Jerdonek, 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 Chris Jerdonek, 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.

Last edited 8 years ago by Chris Jerdonek (previous) (diff)

comment:9 by Aymeric Augustin, 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 Tim Graham, 8 years ago

Has patch: set
Patch needs improvement: set

comment:11 by Chris Jerdonek, 7 years ago

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:12 by Chris Jerdonek, 7 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 Mariusz Felisiak, 2 years ago

Owner: Chris Jerdonek removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top