#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 )
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)
follow-up: 3 comment:1 by , 5 years ago
comment:2 by , 5 years ago
Description: | modified (diff) |
---|
comment:3 by , 5 years ago
comment:4 by , 5 years ago
Cc: | added |
---|---|
Component: | Uncategorized → HTTP handling |
Severity: | Normal → Release blocker |
Summary: | django.db.utils.InterfaceError in test when creating FileResponse with temporary file → FileResponse with temporary file closing connection on PostgreSQL. |
Triage Stage: | Unreviewed → Accepted |
Great catch!
Regression in cce47ff65a4dd3786c049ec14ee889e128ca7de9.
follow-up: 6 comment:5 by , 5 years ago
Resolution: | → invalid |
---|---|
Severity: | Release blocker → Normal |
Status: | new → closed |
Triage Stage: | Accepted → Unreviewed |
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.
comment:6 by , 5 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.
follow-up: 8 comment:7 by , 5 years ago
def setUp(self): request_started.disconnect(close_old_connections) def tearDown(self): request_started.connect(close_old_connections)
comment:8 by , 5 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
follow-up: 10 comment:9 by , 5 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.)
follow-up: 13 comment:10 by , 5 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.
comment:11 by , 5 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
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 , 5 years ago
Cc: | added |
---|
comment:13 by , 5 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 , 5 years ago
Owner: | changed from | to
---|---|
Severity: | Normal → Release blocker |
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
Accepted based on Florian's comment.
comment:15 by , 5 years ago
Summary: | FileResponse with temporary file closing connection on PostgreSQL. → FileResponse with temporary file closing connection. |
---|
comment:16 by , 5 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Oskar, Can you check the ticket number? #30365 is probably not the right one.