Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#22680 closed Bug (fixed)

I/O operation on closed file

Reported by: shelinanton@… Owned by: nobody
Component: File uploads/storage Version: 1.7-beta-2
Severity: Release blocker Keywords:
Cc: Florian Apolloner, Tim Graham, mortas.11@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Save method of file field checks contents size on closed file.

class Author(models.Model):
    photo_url = models.URLField(null=True, blank=True, max_length=1024,verbose_name = _('Original photo url'))
    photo_copy = models.ImageField(upload_to='photos/%Y/%m/%d/authors',null=True, blank=True, verbose_name = _('Thumbnail'))

    def save(self, *args, **kwargs):
        if not self.photo_copy and self.photo_url:
            img_temp = NamedTemporaryFile(delete=True)
            img_temp.write(urllib2.urlopen(self.photo_url).read())
            img_temp.flush()
            self.photo_copy.save('filename.jpg', File(img_temp),save=False)
        super(Author, self).save(*args, **kwargs)

Results in:
ValueError: I/O operation on closed file
django.db.models.fileds.files.py line 93

self._size = content.size

Change History (18)

comment:1 Changed 4 years ago by Aymeric Augustin

Severity: Release blockerNormal

I don't see why this would be a release blocker.

Can you provide the full traceback please? I suspect the exception appears inside File._get_size_from_underlying_file but I'm not sure where exactly.

comment:2 Changed 4 years ago by shelinanton@…

More info could be find here https://groups.google.com/forum/#!topic/django-users/SXMDKFFnoSc
This bug occured just after migration from dajngo 1.6 to django 1.7.
My traceback:

Traceback (most recent call last):
  File "/home/hodza/projects/pinstream/pins/management/commands/import.py", line 85, in handle
    author.save()
  File "/home/hodza/projects/pinstream/pins/models.py", line 199, in save
    self.photo_copy.save(img_filename, File(img_temp),save=False)
  File "/home/hodza/projects/pinstream/venv/src/django/django/db/models/fields/files.py", line 93, in save
    self._size = content.size
  File "/home/hodza/projects/pinstream/venv/src/django/django/core/files/base.py", line 46, in _get_size
    pos = self.file.tell()
ValueError: I/O operation on closed file
Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:3 Changed 4 years ago by shelinanton@…

Created simple script to reproduce error:

from django.db import models
from django.db.models.fields.files import FieldFile
from django.core.files.temp import NamedTemporaryFile
from django.core.files import File
from django.core.files.storage import FileSystemStorage

fs = FileSystemStorage(location='.')
field = models.FileField(storage=fs, name="field")

class Model(object):
    field = ""

instance = Model()
temp = NamedTemporaryFile(delete=True)
field_file = FieldFile(instance,field,"field")
field_file.save('file', File(temp),save=False)

comment:4 Changed 4 years ago by Aymeric Augustin

Severity: NormalRelease blocker

This looks like a regression, I'm marking it as a release blocker to make sure we investigate it before the 1.7 release.

comment:5 Changed 4 years ago by Florian Apolloner

I am not sure that closing files as part of _save is a good choice at all, imo it should be up to the caller to close them (which we currently don't do either). I think we need a mechanism to close stuff from request.FILES at the end of the request.

comment:6 Changed 4 years ago by Florian Apolloner

Cc: Florian Apolloner Tim Graham added

comment:7 Changed 4 years ago by Florian Apolloner

Cc: Tim Graham added; Tim Graham removed

comment:8 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

Original ticket is #21057; commmit: 30fc49a7ca0d030c7855f31ed44395903fa6abdd.

comment:9 Changed 4 years ago by John Hensley

How about instead of setting the cached file size from the intermediate content, we just ask the newly-saved file in the underlying storage? I think that's a simple fix that doesn't require callers to know about and clean up framework debris. The test script above, and of course the test suite, look OK with it.

https://github.com/django/django/pull/2712

comment:10 Changed 4 years ago by Florian Apolloner

@john: That just masks the original issue; I've got a more extensive patch here [As a sideeffect it fixes the ResourceWarnings in file_upload tests since the files are actually closed now :)]: https://github.com/apollo13/django/compare/fdleakage

comment:11 Changed 3 years ago by anonymous

Converted into a PR: https://github.com/django/django/pull/2747/ -- review welcome.

comment:12 Changed 3 years ago by Althalus

Cc: mortas.11@… added

comment:13 Changed 3 years ago by Andrew Godwin

Version: master1.7-beta-2

Changing version so this is obviously a 1.7 blocker.

comment:14 Changed 3 years ago by Tim Graham

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:15 Changed 3 years ago by Florian Apolloner <florian@…>

Resolution: fixed
Status: newclosed

In e2efc8965edf684aaf48621680ef54b84e116576:

Fixed #22680 -- I/O operation on closed file.

This patch is two-fold; first it ensure that Django does close everything in
request.FILES at the end of the request and secondly the storage system should
no longer close any files during save, it's up to the caller to handle that --
or let Django close the files at the end of the request.

comment:16 Changed 3 years ago by Florian Apolloner <florian@…>

In 1ff11304dcebbabdede8ef3d659ca0e54055e2fd:

[1.7.x] Fixed #22680 -- I/O operation on closed file.

This patch is two-fold; first it ensure that Django does close everything in
request.FILES at the end of the request and secondly the storage system should
no longer close any files during save, it's up to the caller to handle that --
or let Django close the files at the end of the request.

Backport of e2efc8965edf684aaf48621680ef54b84e116576 from master.

comment:17 Changed 3 years ago by Loic Bistuer <loic.bistuer@…>

In 6e8d614acd1b65a1ae472da7db88a7b2751dc388:

Made the vendored NamedTemporaryFile work as a context manager. Refs #22680.

This fixes a regression on Windows introduced by b7de5f5.

Thanks Tim Graham for the report and review.

comment:18 Changed 3 years ago by Tim Graham <timograham@…>

In d9eef1f4f7b4401a0ff6e3feb3352b6e661f705a:

[1.7.x] Made the vendored NamedTemporaryFile work as a context manager. Refs #22680.

This fixes a regression on Windows introduced by b7de5f5.

Thanks Tim Graham for the report and review.

Backport of 6e8d614acd from master

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