Opened 2 months ago

Closed 2 months ago

#35618 closed Uncategorized (duplicate)

response.close() in a TestCase prematurely closes PostgreSQL connection and leads to psycopg2.InterfaceError

Reported by: Anders Kaseorg Owned by:
Component: Database layer (models, ORM) Version: 5.0
Severity: Normal Keywords:
Cc: Carlton Gibson, Jon Janzen Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If I call response.close() on an HTTP response from the test client within a django.test.TestCase, Django incorrectly closes the active PostgreSQL connection, causing future test requests to fail with psycopg2.InterfaceError: connection already closed.

This happens because HttpResponseBase.close sends signals.request_finished, which is handled by django.db.close_old_connections, which calls BaseDatabaseWrapper.close_if_unusable_or_obsolete, which observes self.get_autocommit() != self.settings_dict["AUTOCOMMIT"] (False != True) and closes the connection.

Any connection that’s within a transaction (such as the one opened by TestCase) will have get_autocommit() == False. It seems very wrong that the finishing of any request automatically triggers all connections in transactions to be immediately closed.

Minimal complete test project: https://gist.github.com/andersk/b4da5a106995b4c525595ea204675ee0

$ ./manage.py test
Found 1 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
E
======================================================================
ERROR: test (tests.MyTest.test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/db/backends/base/base.py", line 294, in _cursor
    return self._prepare_cursor(self.create_cursor(name))
                                ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/utils/asyncio.py", line 26, in inner
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/db/backends/postgresql/base.py", line 332, in create_cursor
    cursor = self.connection.cursor()
             ^^^^^^^^^^^^^^^^^^^^^^^^
psycopg2.InterfaceError: connection already closed

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/tmp/close-test/tests.py", line 8, in test
    response = self.client.get("/two")
               ^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/test/client.py", line 1049, in get
    response = super().get(path, data=data, secure=secure, headers=headers, **extra)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/test/client.py", line 465, in get
    return self.generic(
           ^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/test/client.py", line 617, in generic
    return self.request(**r)
           ^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/test/client.py", line 1013, in request
    self.check_exception(response)
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/test/client.py", line 743, in check_exception
    raise exc_value
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/my_urls.py", line 7, in view
    with connection.cursor():
         ^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/utils/asyncio.py", line 26, in inner
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/db/backends/base/base.py", line 316, in cursor
    return self._cursor()
           ^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/db/backends/base/base.py", line 293, in _cursor
    with self.wrap_database_errors:
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/db/backends/base/base.py", line 294, in _cursor
    return self._prepare_cursor(self.create_cursor(name))
                                ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/utils/asyncio.py", line 26, in inner
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/close-test/.direnv/python-3.12/lib/python3.12/site-packages/django/db/backends/postgresql/base.py", line 332, in create_cursor
    cursor = self.connection.cursor()
             ^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.InterfaceError: connection already closed

----------------------------------------------------------------------
Ran 1 test in 0.018s

FAILED (errors=1)
Destroying test database for alias 'default'...

Change History (7)

comment:1 by Simon Charette, 2 months ago

In order to prevent this problem from happening under normal circumstances django.test.Client unregisters this signal handler before calling request.close.

I feel like if you're testing something as low level as HttpRequest closing in the context of database connection handling you should be using ClientHandler?

I guess that we could possibly make request.close idempotent given what's returned by test.Client should already be closed?

Version 1, edited 2 months ago by Simon Charette (previous) (next) (diff)

comment:2 by Anders Kaseorg, 2 months ago

The streaming case is the one that motivated me to call response.close in the first place. Currently there’s no way to get closing_iterator_wrapper to close the response itself without first consuming the entire stream, and leaving it unclosed results in various ResourceWarnings when warnings are enabled.

comment:3 by Carlton Gibson, 2 months ago

Cc: Carlton Gibson Jon Janzen added

We had a similar issue in Channels, which we've addressed by (similarly) disabling the close_old_connections handler during tests.

You can see the PR here.

We have this down as related to #30448.

comment:4 by Anders Kaseorg, 2 months ago

Has patch: set

comment:6 by Simon Charette, 2 months ago

Should we close as duplicate of #30448, this seems like the exact same problem.

comment:7 by Carlton Gibson, 2 months ago

Resolution: duplicate
Status: newclosed

Yes, agreed. Duplicate of #30448.

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