connection.is_usable() raises AttributeError after the connection is closed

[Chris Jerdonek]

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.

[Yohann Gabory]

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

[Aymeric Augustin]

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).

[Yohann Gabory]

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

[Chris Jerdonek]

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

[Yohann Gabory]

PR: ​https://github.com/django/django/pull/7114

[Chris Jerdonek]

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.

[Chris Jerdonek]

Also, 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.

[Aymeric Augustin]

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.

[Chris Jerdonek]

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).