Title: cgi.FieldStorage triggers ResourceWarning sometimes
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.4
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Marcel.Hellkamp, barry, brett.cannon, flox, larry, madison.may, orsenthil, pitrou, python-dev, serhiy.storchaka, vajrasky
Priority: release blocker Keywords: 3.3regression

Created on 2013-07-07 09:50 by flox, last changed 2014-01-17 16:04 by brett.cannon. This issue is now closed.

File name Uploaded Description Edit flox, 2013-07-07 09:50
fix_resource_warning_in_test_cgi.patch vajrasky, 2013-08-12 11:22 review
Messages (8)
msg192530 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2013-07-07 09:50
It happens when POSTing a file for example.

When running the test suite:
./python.exe -m test test_cgi

Or with the script attached:
$ ./python ResourceWarning: unclosed file <_io.BufferedRandom name=3>
  check('x' * 1010)           # ResourceWarning ResourceWarning: unclosed file <_io.BufferedRandom name=3>
  check('x' * (maxline - 1))  # ResourceWarning
msg194940 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-12 11:22
It happens because if the length of data is more than 1000:

    def __write(self, line):
        """line is always bytes, not string"""
        if self.__file is not None:
            if self.__file.tell() + len(line) > 1000:
                self.file = self.make_file()
                data = self.__file.getvalue()
                self.__file = None

it will create a temporary file.

Attached the patch to close the temporary file in the destructor method.

About the 1000 number, should we put it in constant? Why 1000? This number is so random. For now, I just leave it as it is.
msg194980 - (view) Author: Madison May (madison.may) * Date: 2013-08-12 18:16
I ran into a similar issue (see #18700) with test_cgi.

``/home/mmay/cpython/Lib/test/ ResourceWarning: unclosed file <_io.BufferedRandom name=3>``
msg196009 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-08-23 19:15
New changeset c0e9ba7b26d5 by Brett Cannon in branch 'default':
Issue #18394: Explicitly close the file object cgi.FieldStorage
msg196010 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-08-23 19:17
I managed to write a similar patch to Vajrasky independently, so at least I know the approach is reasonable. =)

I'm not backporting since it really isn't that critical; I fixed it just to turn off the ResourceWarning while running the test suite.
msg207958 - (view) Author: Marcel Hellkamp (Marcel.Hellkamp) Date: 2014-01-12 14:32
This change breaks existing applications.

The cgi.FieldStorage.file attribute is public and mentioned in the documentation. It even states "You can then read the data at leisure from the file attribute".

Consider this example::

    form = cgi.FieldStorage()
    fileitem = form.getfirst("userfile")
    if fileitem and fileitem.file:
        executor.submit(store_file, fileitem.file, fileitem.filename)

This code is no longer safe. The garbage collector might close the file handle while it is still referenced and accessed from the worker thread.

Another example is the bottle web framework. It uses cgi.FieldStorage for parsing only, extracts the valuable information and stores the result in its own data structures. The cgi.FieldStorage instance is lost. Python 3.4 breaks every single bottle application that works with file uploads.

How about implementing the context manager protocol for cgi.FieldStorage to resolve this issue?
msg208339 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-17 14:49
While you're right, Marcel, that code which pulls out the file object form FieldStorage would probably have the file closed from underneath it, I don't know if I agree that it's a bad thing. The FieldStorage object created that file, implicitly putting it in charge of managing it. Having it guarantee that it gets closed seems to me totally reasonable and a bug not to do so. And having Bottle break as-is doesn't sway me as this is a bug fix and so updating Bottle as part of the process to support Python 3.4 is reasonable.

That use of the term "leisure" is definitely a problem, though. So I'm going to make this a bug report for updating the docs to no longer use the term "leisure" and add a versionchanged note that FieldStorage will close the 'file' attribute upon deletion. I'll also opened to add context manager support to FieldStorage.

But since leaving files randomly open is not good I can't bring myself to revert this change.
msg208343 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-01-17 16:03
New changeset 13d04a8713ad by Brett Cannon in branch 'default':
Issue #18394: Document that cgi.FieldStorage now cleans up after its
Date User Action Args
2014-01-17 16:04:56brett.cannonsetstatus: open -> closed
2014-01-17 16:03:27python-devsetmessages: + msg208343
2014-01-17 14:49:14brett.cannonsetmessages: + msg208339
2014-01-12 17:21:55brett.cannonsetstatus: closed -> open

nosy: + larry
priority: normal -> release blocker
assignee: brett.cannon
keywords: + 3.3regression, - patch
2014-01-12 14:32:46Marcel.Hellkampsetnosy: + Marcel.Hellkamp
messages: + msg207958
2013-08-23 19:17:25brett.cannonsetstatus: open -> closed
versions: - Python 3.3
nosy: + brett.cannon

messages: + msg196010

resolution: fixed
2013-08-23 19:15:57python-devsetnosy: + python-dev
messages: + msg196009
2013-08-13 13:31:22serhiy.storchakasetnosy: + pitrou
2013-08-12 18:16:30madison.maysetnosy: + madison.may
messages: + msg194980
2013-08-12 11:22:20vajraskysetfiles: + fix_resource_warning_in_test_cgi.patch

nosy: + vajrasky
messages: + msg194940

keywords: + patch
2013-07-07 14:14:39barrysetnosy: + barry
2013-07-07 09:50:01floxcreate