Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#31240 closed Bug (fixed)

FileResponse with temporary file closing connection.

Reported by: Oskar Persson Owned by: Florian Apolloner
Component: HTTP handling Version: 3.0
Severity: Release blocker Keywords:
Cc: Carlton Gibson, Chris Jerdonek, Florian Apolloner Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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

Change History (18)

comment:1 by Mariusz Felisiak, 4 years ago

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

comment:2 by Oskar Persson, 4 years ago

Description: modified (diff)

in reply to:  1 comment:3 by Oskar Persson, 4 years ago

Replying to felixxm:

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

Fixed

comment:4 by Mariusz Felisiak, 4 years ago

Cc: Carlton Gibson Chris Jerdonek added
Component: UncategorizedHTTP handling
Severity: NormalRelease blocker
Summary: django.db.utils.InterfaceError in test when creating FileResponse with temporary fileFileResponse with temporary file closing connection on PostgreSQL.
Triage Stage: UnreviewedAccepted

Great catch!

Regression in cce47ff65a4dd3786c049ec14ee889e128ca7de9.

comment:5 by Mariusz Felisiak, 4 years ago

Resolution: invalid
Severity: Release blockerNormal
Status: newclosed
Triage Stage: AcceptedUnreviewed

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.

in reply to:  5 comment:6 by Oskar Persson, 4 years ago

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.

comment:7 by Carlton Gibson, 4 years ago

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

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

in reply to:  7 comment:8 by Oskar Persson, 4 years ago

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

comment:9 by Carlton Gibson, 4 years ago

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

in reply to:  9 ; comment:10 by Oskar Persson, 4 years ago

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.

Last edited 4 years ago by Oskar Persson (previous) (diff)

comment:11 by Carlton Gibson, 4 years ago

Resolution: invalid
Status: closednew

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.

comment:12 by Mariusz Felisiak, 4 years ago

Cc: Florian Apolloner added

in reply to:  10 comment:13 by Florian Apolloner, 4 years ago

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

comment:14 by Mariusz Felisiak, 4 years ago

Owner: changed from nobody to Florian Apolloner
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: UnreviewedAccepted

Accepted based on Florian's comment.

comment:15 by Mariusz Felisiak, 4 years ago

Summary: FileResponse with temporary file closing connection on PostgreSQL.FileResponse with temporary file closing connection.

comment:16 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 41a3b3d:

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

Thanks to Oskar Persson for the report.

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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

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