Opened 3 years ago

Closed 3 years ago

#33023 closed Bug (wontfix)

InMemoryUploadedFile - opening with context manager multiple time is throwing error

Reported by: Harsh Bhikadia Owned by: nobody
Component: Core (Other) Version: 3.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Harsh Bhikadia)

What is the issue:
when "opening" the "uploaded file" with "context manager (like with file.open('rb') as f:) multiple times it is throwing ValueError: I/O operation on closed file. exception

Explaination
Digging into the code, I realised that InMemoryUploadedFile overrides the open method from File instance - the implementation just "seeks to 0". Since the context manager will call the close method on "exit". When "opening" the file again (with or without context manager) it fails with "cannot seek(0) of already closed file"

Potential solution

  • Not sure why the "open" method was overridden - if not required then just should remove the "overridden method"
  • or making sure the file is "re-opened" (like in File) if closed

To conclude

Since the doc on `File` mentions that it could be used with "context manager" - I think this is unexpected behaviour and should be fixed.

I have been using Django for many years but new to contribution/issue reporting, so forgive me when I am being ignorant.

I am interested in fixing this myself if someone here gives me a green signal for it.

Change History (4)

comment:1 by Harsh Bhikadia, 3 years ago

Description: modified (diff)
Resolution: fixed
Status: newclosed

comment:2 by Harsh Bhikadia, 3 years ago

Also realised the implementation is not respecting the mode passed in arg when calling open.

The current implementation (master branch as of now) looks like the following:

class InMemoryUploadedFile(UploadedFile):
    """
    A file uploaded into memory (i.e. stream-to-memory).
    """
    def __init__(self, file, field_name, name, content_type, size, charset, content_type_extra=None):
        super().__init__(file, name, content_type, size, charset, content_type_extra)
        self.field_name = field_name

    def open(self, mode=None):
        self.file.seek(0)
        return self

    def chunks(self, chunk_size=None):
        self.file.seek(0)
        yield self.read()

    def multiple_chunks(self, chunk_size=None):
        # Since it's in memory, we'll never have multiple chunks.
        return False

comment:3 by Harsh Bhikadia, 3 years ago

Resolution: fixed
Status: closednew

comment:4 by Carlton Gibson, 3 years ago

Resolution: wontfix
Status: newclosed

Hi. Thanks for the report.

I don't think it's worth complicating the UploadedFile code here. It's a way of Django passing the file data to your application. If you need to process that data multiple times you should save it to a suitable location (which may be in memory) and then use that, which is fully under your control. (That the file will be closed on exiting the context manager allows the memory to be released. That's the right behaviour, that we'd want to maintain.)

Not sure why the "open" method was overridden

It's required to ensure the BytesIO instance is ready to read. It's been there for many years, at least since the current file upload structure was introduced. See #2070 (d725cc9734272f867d41f7236235c28b3931a1b2).

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