classification
Title: tempfile.TemporaryFile and httplib incompatibility
Type: behavior Stage: resolved
Components: Windows Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Ramchandra Apte, demian.brecht, ishimoto, lemanyk1, martin.panter, orsenthil, python-dev, r.david.murray, serhiy.storchaka, tim.golden, tzs, vstinner
Priority: normal Keywords: needs review, patch

Created on 2012-07-07 00:24 by tzs, last changed 2015-05-16 16:07 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
issue15267.patch ishimoto, 2012-07-30 08:29 review
Messages (14)
msg164764 - (view) Author: Tim Smith (tzs) Date: 2012-07-07 00:24
In httplib.py, there is this code to try to get the body length:

    def _set_content_length(self, body):
        # Set the content-length based on the body.
        thelen = None
        try:
            thelen = str(len(body))
        except TypeError, te:
            # If this is a file-like object, try to
            # fstat its file descriptor
            try:
                thelen = str(os.fstat(body.fileno()).st_size)
            except (AttributeError, OSError):
                # Don't send a length if this failed
                if self.debuglevel > 0: print "Cannot stat!!"

However, if the body is a temporary file created by tempfile.TemporaryFile(), the len(body) in the first try throws an AttributeError, not a TypeError, on Windows and so it is not caught and unhappiness ensues. It is fine on Macintosh, and I would presume also on Linux.

Windows behaves different because on the other systems, TemporaryFile() returns an actual file object, and len() on a file throws TypeError. On Windows, TemporaryFile() returns an object that wraps the underlying file, and calling len() on that objects invokes this from tempfile.py (around line 371):

    def __getattr__(self, name):
        # Attribute lookups are delegated to the underlying file
        # and cached for non-numeric results
        # (i.e. methods are cached, closed and friends are not)
        file = self.__dict__['file']
        a = getattr(file, name)
        if not issubclass(type(a), type(0)):
            setattr(self, name, a)
        return a

Since the underlying file does not have a __len__ method, the getattr fails and throws AttributeError.

I'm sorry I'm not submitting a patch, but I do not know enough about the design of these two libraries to know whether the correct fix is for httplib to be more broad in the exceptions that cause it to check for a file when len() fails, or if the object returned by TemporaryFile() should be more closely mimicking a true file object and so should be made to throw TypeError when someone tries to use len() on it.
msg165074 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2012-07-09 10:31
Could you create a failing test, please, Tim S?
msg165076 - (view) Author: Ramchandra Apte (Ramchandra Apte) * Date: 2012-07-09 11:36
BTW, type(0) should be replaced with int in the code.
msg165093 - (view) Author: Tim Smith (tzs) Date: 2012-07-09 16:19
Here is a program that demonstrates the problem:

    import httplib
    import tempfile

    f = tempfile.TemporaryFile()
    f.write("Hello, Temporary File!")
    f.seek(0)

    c = httplib.HTTPConnection('bugs.python.org')
    c.request('POST', '/', f, {'Content-type' : 'application/octet-stream'})

    r = c.getresponse()
    print r.status, r.reason

The expected output is "200 OK", and that's what happens on Mac and almost certainly on Linux. On Windows, this is the result:

    Traceback (most recent call last):
      File "bad.py", line 9, in <module>
        c.request('POST', '/', f, {'Content-type' : 'application/octet
      File "C:\Python27\lib\httplib.py", line 958, in request
        self._send_request(method, url, body, headers)
      File "C:\Python27\lib\httplib.py", line 989, in _send_request
        self._set_content_length(body)
      File "C:\Python27\lib\httplib.py", line 964, in _set_content_len
        thelen = str(len(body))
      File "C:\Python27\lib\tempfile.py", line 383, in __getattr__
        a = getattr(file, name)
    AttributeError: 'file' object has no attribute '__len__'

Changing the lines:

    f = tempfile.TemporaryFile()
    f.write("Hello, Temporary File!")
    f.seek(0)

to:

    f = open("temp.file", "w+")
    f.write("Hello, Less Temporary File!")
    f.seek(0)

makes it work on Windows.
msg165183 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2012-07-10 10:14
I can confirm that this isn't a problem for 3.x. No-one's listed in the experts list for the httplib module but Senthil appears to be the de factor maintainer so I've added him to the call for his opinion. My feeling would be to do the simplest thing possible and add AttributeError to the list of trapped exceptions.
msg166867 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-30 08:29
patch contains fix and test for 2.7.
With this patch, AttibuteError is captured as Tim sujested.
msg189285 - (view) Author: (lemanyk1) Date: 2013-05-15 15:12
i think it is not good, len(fd) must raise AttributeError, not TypeError
msg232533 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-12 12:12
Catching TypeError on len() looks as an ugly test to check if body is a string. Why not doing the opposite: first to call fileno() and call AttributeError? Or even use hasattr(body, "fileno")?
msg232534 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-12-12 12:14
issue15267.patch: I would feel more confortable if test_send_tempfile() ensures that the socket contains the file content.
msg240469 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-04-11 11:26
See also Issue 23740, about cleaning up the types for computing Content-Length in 3.5.
msg240470 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-11 11:46
More general and simple solution is to make tempfile.NamedTemporaryFile new-style class.

Old-style class:

>>> import tempfile
>>> f = tempfile.NamedTemporaryFile()
>>> len(f)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/serhiy/py/cpython-2.7/Lib/tempfile.py", line 391, in __getattr__
    a = getattr(file, name)
AttributeError: 'file' object has no attribute '__len__'

New-style class:

>>> import tempfile
>>> f = tempfile.NamedTemporaryFile()
>>> len(f)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: object of type '_TemporaryFileWrapper' has no len()
msg240481 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-11 15:17
We don't do classic to new style class changes for bug fixes without a compelling reason; we've been bitten by unexpected breakage doing that in the past.
msg243327 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-16 15:59
New changeset c34513c2a894 by Serhiy Storchaka in branch '2.7':
Issue #15267: HTTPConnection.request() now is compatibile with old-style
https://hg.python.org/cpython/rev/c34513c2a894
msg243328 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-16 16:07
Catching TypeError or AttributeError on len() is a hack, but it is compatible with Python 3 and older code. We could handle this in issue23740.

Changing TemporaryFile would solve this issue, but only for TemporaryFile. len() raises AttributeError for other file-like classic classes. So I committed slightly modified Atsuo's patch (extended test as Victor suggested).

As for converting classic classes to new-style classes, see for example issue14399. It is not so harmless, as expected, unfortunately. But perhaps it was done multiple times in bug-fix releases.
History
Date User Action Args
2015-05-16 16:07:44serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg243328

stage: patch review -> resolved
2015-05-16 15:59:16python-devsetnosy: + python-dev
messages: + msg243327
2015-05-16 15:50:00serhiy.storchakasetassignee: serhiy.storchaka
2015-04-11 15:17:40r.david.murraysetnosy: + r.david.murray
messages: + msg240481
2015-04-11 11:46:45serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg240470
2015-04-11 11:26:14martin.pantersetnosy: + martin.panter
messages: + msg240469
2014-12-12 12:14:03vstinnersetmessages: + msg232534
2014-12-12 12:12:05vstinnersetnosy: + vstinner
messages: + msg232533
2014-12-12 07:59:34serhiy.storchakasetkeywords: + needs review
stage: patch review
2014-07-23 23:50:48demian.brechtsetnosy: + demian.brecht
2013-05-15 15:12:24lemanyk1setnosy: + lemanyk1
messages: + msg189285
2012-07-30 08:29:14ishimotosetfiles: + issue15267.patch

nosy: + ishimoto
messages: + msg166867

keywords: + patch
2012-07-10 10:14:22tim.goldensetnosy: + orsenthil
messages: + msg165183
2012-07-09 16:19:53tzssetmessages: + msg165093
2012-07-09 11:36:19Ramchandra Aptesetnosy: + Ramchandra Apte
messages: + msg165076
2012-07-09 10:31:52tim.goldensetnosy: + tim.golden
messages: + msg165074
2012-07-07 00:24:34tzscreate