[Django] #31240: django.db.utils.InterfaceError in test when creating FileResponse with temporary file
Django
temporary file
-----------------------------------------+------------------------
Reporter: Oskar Persson | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
I think I might've found a regression in #30365. 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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31240>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
temporary file
Reporter: Oskar Persson | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Oskar, Can you check the ticket number? #30365 is probably not the right
one.
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:1>
Django
temporary file
Reporter: Oskar Persson | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Oskar Persson:
Old description:
New description:
I think I might've found a regression in #30565. When I run the following
User = get_user_model()
def test_second(self):
pass
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:2>
Django
temporary file
Reporter: Oskar Persson | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Oskar Persson):
Replying to [comment:1 felixxm]:
> Oskar, Can you check the ticket number? #30365 is probably not the right
one.
Fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:3>
Django
---------------------------------+------------------------------------
Reporter: Oskar Persson | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Carlton Gibson, Chris Jerdonek (added)
* component: Uncategorized => HTTP handling
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Great catch!
Regression in cce47ff65a4dd3786c049ec14ee889e128ca7de9.
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:4>
Django
Reporter: Oskar Persson | Owner: nobody
Component: HTTP handling | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => invalid
* severity: Release blocker => Normal
* stage: Accepted => Unreviewed
Comment:
Sorry for the previous approval. It is an expected behavior, connection
should be closed when we close a response. You have to
[https://github.com/django/django/blob/master/tests/wsgi/tests.py#L15-L19
disconnect signals] to not close connections.
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:5>
Django
-------------------------------+--------------------------------------
Component: HTTP handling | Version: 3.0
Severity: Normal | Resolution: invalid
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Oskar Persson):
Replying to [comment:5 felixxm]:
> Sorry for the previous approval. It is an expected behavior, connection
should be closed when we close a response. You have to
[https://github.com/django/django/blob/master/tests/wsgi/tests.py#L15-L19
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:6>
Django
-------------------------------+--------------------------------------
Component: HTTP handling | Version: 3.0
Severity: Normal | Resolution: invalid
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
{{{
def setUp(self):
request_started.disconnect(close_old_connections)
def tearDown(self):
request_started.connect(close_old_connections)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:7>
Django
-------------------------------+--------------------------------------
Component: HTTP handling | Version: 3.0
Severity: Normal | Resolution: invalid
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Oskar Persson):
Replying to [comment:7 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
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:8>
Django
-------------------------------+--------------------------------------
Component: HTTP handling | Version: 3.0
Severity: Normal | Resolution: invalid
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:9>
Django
-------------------------------+--------------------------------------
Component: HTTP handling | Version: 3.0
Severity: Normal | Resolution: invalid
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Oskar Persson):
Replying to [comment:9 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 [https://github.com/django/django-docker-box/ django-docker-
box]. I'm 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:10>
Django
-------------------------------+--------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: closed => new
* resolution: invalid =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:11>
Django
-------------------------------+--------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Florian Apolloner (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:12>
Django
-------------------------------+--------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Florian Apolloner):
Replying to [comment:10 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:13>
Django
Reporter: Oskar Persson | Owner: Florian
| Apolloner
Type: Bug | Status: assigned
Component: HTTP handling | Version: 3.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* owner: nobody => Florian Apolloner
* status: new => assigned
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Accepted based on Florian's comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Oskar Persson | Owner: Florian
| Apolloner
Type: Bug | Status: assigned
Component: HTTP handling | Version: 3.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Oskar Persson | Owner: Florian
| Apolloner
Type: Bug | Status: assigned
Component: HTTP handling | Version: 3.0
Severity: Release blocker | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Oskar Persson | Owner: Florian
| Apolloner
Component: HTTP handling | Version: 3.0
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"41a3b3d18647b258331104520e76f977406c590d" 41a3b3d]:
{{{
#!CommitTicketReference repository=""
revision="41a3b3d18647b258331104520e76f977406c590d"
Fixed #31240 -- Properly closed FileResponse when wsgi.file_wrapper is
used.
Thanks to Oskar Persson for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Oskar Persson | Owner: Florian
| Apolloner
Type: Bug | Status: closed
Component: HTTP handling | Version: 3.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4e8d6a1bafbbebc0720c5d1ed751521356364fe9" 4e8d6a1b]:
{{{
#!CommitTicketReference repository=""
revision="4e8d6a1bafbbebc0720c5d1ed751521356364fe9"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31240#comment:18>