FileResponse with temporary file closing connection.

[Oskar Persson]

I think I might've found a regression in #30565. When I run the following tests (in their defined order) against Postgres I get the error below.

import tempfile
from django.contrib.auth import get_user_model
from django.http import FileResponse
from django.test import TestCase

User = get_user_model()

class MyTests(TestCase):
    def setUp(self):
        self.user = User.objects.create(username='user')

    def test_first(self):
        with tempfile.TemporaryFile() as f:
            return FileResponse(f)

    def test_second(self):
        pass

Running tests...
----------------------------------------------------------------------
.E
======================================================================
ERROR [0.003s]: test_second (responses.test_fileresponse.MyTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tests/django/django/db/backends/base/base.py", line 238, in _cursor
    return self._prepare_cursor(self.create_cursor(name))
  File "/tests/django/django/utils/asyncio.py", line 26, in inner
    return func(*args, **kwargs)
  File "/tests/django/django/db/backends/postgresql/base.py", line 231, 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 "/tests/django/tests/responses/test_fileresponse.py", line 19, in setUp
    self.user = User.objects.create(username='user')
  File "/tests/django/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/tests/django/django/db/models/query.py", line 433, in create
    obj.save(force_insert=True, using=self.db)
  File "/tests/django/django/contrib/auth/base_user.py", line 66, in save
    super().save(*args, **kwargs)
  File "/tests/django/django/db/models/base.py", line 746, in save
    force_update=force_update, update_fields=update_fields)
  File "/tests/django/django/db/models/base.py", line 784, in save_base
    force_update, using, update_fields,
  File "/tests/django/django/db/models/base.py", line 887, in _save_table
    results = self._do_insert(cls._base_manager, using, fields, returning_fields, raw)
  File "/tests/django/django/db/models/base.py", line 926, in _do_insert
    using=using, raw=raw,
  File "/tests/django/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/tests/django/django/db/models/query.py", line 1204, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
  File "/tests/django/django/db/models/sql/compiler.py", line 1382, in execute_sql
    with self.connection.cursor() as cursor:
  File "/tests/django/django/utils/asyncio.py", line 26, in inner
    return func(*args, **kwargs)
  File "/tests/django/django/db/backends/base/base.py", line 260, in cursor
    return self._cursor()
  File "/tests/django/django/db/backends/base/base.py", line 238, in _cursor
    return self._prepare_cursor(self.create_cursor(name))
  File "/tests/django/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/tests/django/django/db/backends/base/base.py", line 238, in _cursor
    return self._prepare_cursor(self.create_cursor(name))
  File "/tests/django/django/utils/asyncio.py", line 26, in inner
    return func(*args, **kwargs)
  File "/tests/django/django/db/backends/postgresql/base.py", line 231, in create_cursor
    cursor = self.connection.cursor()
django.db.utils.InterfaceError: connection already closed

[Mariusz Felisiak]

Oskar, Can you check the ticket number? #30365 is probably not the right one.

[Oskar Persson]

Replying to felixxm:

Oskar, Can you check the ticket number? #30365 is probably not the right one.

Fixed

[Mariusz Felisiak]

Great catch!

Regression in cce47ff65a4dd3786c049ec14ee889e128ca7de9.

[Mariusz Felisiak]

Sorry for the previous approval. It is an expected behavior, connection should be closed when we close a response. You have to ​disconnect signals to not close connections.

[Oskar Persson]

Replying to felixxm:

Sorry for the previous approval. It is an expected behavior, connection should be closed when we close a response. You have to ​disconnect signals to not close connections.

Hmm, okay. What would that look like in my example? Also, I've now tested this with MySQL as well which gives me the following error:

Running tests...
----------------------------------------------------------------------
.E
======================================================================
ERROR [0.002s]: test_second (responses.test_fileresponse.MyTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tests/django/tests/responses/test_fileresponse.py", line 19, in setUp
    self.user = User.objects.create(username='user')
  File "/tests/django/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/tests/django/django/db/models/query.py", line 433, in create
    obj.save(force_insert=True, using=self.db)
  File "/tests/django/django/contrib/auth/base_user.py", line 66, in save
    super().save(*args, **kwargs)
  File "/tests/django/django/db/models/base.py", line 746, in save
    force_update=force_update, update_fields=update_fields)
  File "/tests/django/django/db/models/base.py", line 784, in save_base
    force_update, using, update_fields,
  File "/tests/django/django/db/models/base.py", line 887, in _save_table
    results = self._do_insert(cls._base_manager, using, fields, returning_fields, raw)
  File "/tests/django/django/db/models/base.py", line 926, in _do_insert
    using=using, raw=raw,
  File "/tests/django/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/tests/django/django/db/models/query.py", line 1204, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
  File "/tests/django/django/db/models/sql/compiler.py", line 1384, in execute_sql
    cursor.execute(sql, params)
  File "/tests/django/django/db/backends/utils.py", line 68, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/tests/django/django/db/backends/utils.py", line 77, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/tests/django/django/db/backends/utils.py", line 80, in _execute
    self.db.validate_no_broken_transaction()
  File "/tests/django/django/db/backends/base/base.py", line 449, in validate_no_broken_transaction
    "An error occurred in the current transaction. You can't "
django.db.transaction.TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.

[Carlton Gibson]

    def setUp(self):
        request_started.disconnect(close_old_connections)

    def tearDown(self):
        request_started.connect(close_old_connections)

[Oskar Persson]

Replying to Carlton Gibson:

    def setUp(self):
        request_started.disconnect(close_old_connections)

    def tearDown(self):
        request_started.connect(close_old_connections)

I'm still getting the same error

[Carlton Gibson]

Without looking at actual code it's hard to say but... :)

The close() call triggers the signal to clean up the DB connections. So if that signal is disconnected for the test, it won't fire.
Or that's the theory.

If you can put together a minimal reproduce project (including the signal detachment) I'm happy to look at it next week.
(Please reopen if you do.)

[Oskar Persson]

Replying to Carlton Gibson:

Without looking at actual code it's hard to say but... :)

The close() call triggers the signal to clean up the DB connections. So if that signal is disconnected for the test, it won't fire.
Or that's the theory.

If you can put together a minimal reproduce project (including the signal detachment) I'm happy to look at it next week.
(Please reopen if you do.)

Sure I could, but I mean the only relevant part really is the code I've already provided which I simply stuck into the top of tests/responses/test_fileresponse.py in the Django repo and ran the test suite using ​django-docker-box.

I'm just not sure I understand how it is expected behaviour that using a temporary file in a FileResponse should disrupt any DB connections? The error raised when testing against MySQL/MariaDB is especially confusing when it says an error occurred inside a transaction.

And another thing to note is that if I create the response using FileResponse(file) instead of the temporary file, the error disappears.

[Carlton Gibson]

OK, I’ll reopen to double check. Either Mariusz will get to it again or, I’ll look into it next week.
Thanks for the report.

[Florian Apolloner]

Replying to Oskar Persson:

I'm just not sure I understand how it is expected behaviour that using a temporary file in a FileResponse should disrupt any DB connections? The error raised when testing against MySQL/MariaDB is especially confusing when it says an error occurred inside a transaction.

Because closing a filelike passed to FileResponse like this will trigger the end of a request which will as part of that also close database connections. This was apparently added in ​https://github.com/django/django/commit/cce47ff65a4dd3786c049ec14ee889e128ca7de9 to fix a real problem, but imo the fix operates at the wrong level.

And another thing to note is that if I create the response using FileResponse(__file__) instead of the temporary file, the error disappears.

This makes sense since __file__ is an open file descriptor which does not get closed here. The problem is that the closing of the TemporaryFile causes the patched close method of the filelike to trigger the close of the FileResponse -- this should not happen and is where ​https://github.com/django/django/commit/cce47ff65a4dd3786c049ec14ee889e128ca7de9 goes wrong (it should probably only do this magic if file_wrapper is actually used).

[Mariusz Felisiak]

Accepted based on Florian's comment.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 41a3b3d:

Fixed #31240 -- Properly closed FileResponse when wsgi.file_wrapper is used.

Thanks to Oskar Persson for the report.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4e8d6a1b:

[3.0.x] Fixed #31240 -- Properly closed FileResponse when wsgi.file_wrapper is used.

Thanks to Oskar Persson for the report.

Backport of 41a3b3d18647b258331104520e76f977406c590d from master