[Django] #29278: FileResponse documentation should warn against using context managers
Django
------------------------------------------+------------------------
Reporter: Mike DePalatis | Owner: nobody
Type: Uncategorized | Status: new
Component: Documentation | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------+------------------------
Idiomatic Python usually opens file objects using a context manager like
this:
{{{
with open("filename.txt", "rb") as fo:
# do stuff with fo
}}}
This is problematic when using a `FileResponse` which requires a file-like
object rather than a path. Doing something like:
{{{
with open("filename.txt", "rb") as fo:
return FileResponse(fo)
}}}
results in an error due to trying to operate on a closed file:
{{{
Exception happened during processing of request from ('127.0.0.1', 60169)
Traceback (most recent call last):
File
"/Users/zduey/anaconda3/envs/cml/lib/python3.5/wsgiref/handlers.py", line
138, in run
self.finish_response()
File
"/Users/zduey/anaconda3/envs/cml/lib/python3.5/wsgiref/handlers.py", line
179, in finish_response
for data in self.result:
File "/Users/zduey/anaconda3/envs/cml/lib/python3.5/wsgiref/util.py",
line 30, in __next__
data = self.filelike.read(self.blksize)
ValueError: read of closed file
}}}
The documentation for `FileResponse` should be updated to explicitly warn
users against passing in file objects opened with a context manager and
make clear that the file will be closed automatically.
--
Ticket URL: <https://code.djangoproject.com/ticket/29278>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Mike DePalatis | Owner: nobody
Component: Documentation | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Claude Paroz):
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:1>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 2.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Windson yang):
* owner: nobody => Windson yang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:2>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Windson yang):
[https://github.com/django/django/pull/9847 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:3>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by andrei kulakov):
I don't think this requires a warning because the sole function and
purpose of `with` construct is to close file objects / clean up state when
done with the block. By the same logic we would have to document all
Python features in Django docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:4>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Mike DePalatis):
Replying to [comment:4 andrei kulakov]:
> I don't think this requires a warning because the sole function and
purpose of `with` construct is to close file objects / clean up state when
done with the block. By the same logic we would have to document all
Python features in Django docs.
I don't think all Python features need to be documented. However, this is
a special case where the required way of opening a file goes against the
usual best practices and thus it makes sense that this should be
explicitly noted.
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:5>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by andrei kulakov):
Replying to [comment:5 Mike DePalatis]:
>
> I don't think all Python features need to be documented. However, this
is a special case where the required way of opening a file goes against
the usual best practices and thus it makes sense that this should be
explicitly noted.
In theory, perhaps, but in this case someone using this best practice
needs to understand the reason for 'with' construct. It's a fairly common
construct and its
use is much more general than just file objects. Also the way it works is
neither very obscure nor complex to understand.
You could also argue that the best practice is "use with construct and
complete all work on file within the `with` block", and so this best
practice does not apply
here because obviously no work is completed in the example.
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:6>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Claude Paroz):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
I think it's useful for users to know `FileResponse` will close the file
itself, and that it's not possible to close the file before the response
has finished streaming it.
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:7>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Component: Documentation | Version: 2.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4fe5d846666d46a5395a5f0ea2845a96b6837a75" 4fe5d846]:
{{{
#!CommitTicketReference repository=""
revision="4fe5d846666d46a5395a5f0ea2845a96b6837a75"
Fixed #29278 -- Doc'd that a context manager can't be used with
FileResponse.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:8>
Django
Reporter: Mike DePalatis | Owner: Windson
Type: | yang
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a390cc710939365e58e6dc98ec282e532f71a3f9" a390cc71]:
{{{
#!CommitTicketReference repository=""
revision="a390cc710939365e58e6dc98ec282e532f71a3f9"
[2.0.x] Fixed #29278 -- Doc'd that a context manager can't be used with
FileResponse.
Backport of 4fe5d846666d46a5395a5f0ea2845a96b6837a75 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29278#comment:9>