Opened 10 years ago

Closed 10 years ago

Last modified 10 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 by Aymeric Augustin, 10 years ago

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 by shelinanton@…, 10 years ago

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 10 years ago by Aymeric Augustin (previous) (diff)

comment:3 by shelinanton@…, 10 years ago

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 by Aymeric Augustin, 10 years ago

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 by Florian Apolloner, 10 years ago

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 by Florian Apolloner, 10 years ago

Cc: Florian Apolloner Tim Graham added

comment:7 by Florian Apolloner, 10 years ago

Cc: Tim Graham added; Tim Graham removed

comment:8 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

Original ticket is #21057; commmit: 30fc49a7ca0d030c7855f31ed44395903fa6abdd.

comment:9 by John Hensley, 10 years ago

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 by Florian Apolloner, 10 years ago

@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 by anonymous, 10 years ago

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

comment:12 by Althalus, 10 years ago

Cc: mortas.11@… added

comment:13 by Andrew Godwin, 10 years ago

Version: master1.7-beta-2

Changing version so this is obviously a 1.7 blocker.

comment:14 by Tim Graham, 10 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:15 by Florian Apolloner <florian@…>, 10 years ago

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 by Florian Apolloner <florian@…>, 10 years ago

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 by Loic Bistuer <loic.bistuer@…>, 10 years ago

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 by Tim Graham <timograham@…>, 10 years ago

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