classification
Title: AttributeError: 'NoneType' in http/client.py when using select when file descriptor is closed.
Type: enhancement Stage: needs patch
Components: Documentation Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: docs@python Nosy List: docs@python, fviard, pitrou, r.david.murray, terry.reedy
Priority: normal Keywords: easy, patch

Created on 2013-10-03 17:12 by fviard, last changed 2013-11-05 02:54 by terry.reedy.

Messages (11)
msg198902 - (view) Author: Florent Viard (fviard) Date: 2013-10-03 17:12
In Lib/http/client.py +682    (Formerly httplib)

    def fileno(self):
        return self.fp.fileno()

This function should be modified to be able to handle the case where the http request is already completed and so "fp" is closed. Ex.:
    def fileno(self):
        if self.fp:
            return self.fp.fileno()
        else:
            return -1

I encountered the issue in the following context:
while 1:
  read_list = select([req], ...)[0]
  if read_list:
    req.read(CHUNK_SIZE)
  ...

Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nappstore/server_comm.py", line 211, in download_file
    ready = select.select([req], [], [], timeout)[0]
  File "/usr/lib/python2.7/socket.py", line 313, in fileno
    return self._sock.fileno()
  File "/usr/lib/python2.7/httplib.py", line 655, in fileno
    return self.fp.fileno()
  AttributeError: 'NoneType' object has no attribute 'fileno'

For the returned value, I'm not sure because there is currently 2 different cases for other objects returning a fileno. 
In Lib/fileinput.py:
  -1 is returned in case of ValueError (no fileno value as fp was closed)
but in Lib/socket.py:
  ValueError is raised in that case and default value for fileno for a socket is None
msg198979 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-10-05 02:55
Changing the API of HTTPResponse.fileno is a feature change, not a bug fix. I do not think it should be changed as it would break existing code to no purpose. Instead, calls should continue to wrapped in try: except ValueError when failure is possible. And this issue closed as rejected.

--
As to the illustrative example: 

I do not think the call in _fileobject.fileno should be so wrapped because I think the bug is elsewhere. _fileinput()._sock is supposed to be a socket, not an HTTPResponse. This says to me that a) self._sock.fileno() is supposed to call socket.fileno, not HTTPResonse.fileno; and b) nappstore/server_comm.py is calling socket(_sock=<HTTPResonse object>) (or directly modifying the .sock attribute).

If b) is true, that strikes me as a bug because _sock is an undocumented private parameter, subject to change. Indeed it disappeared in 3.x when socket became a _socket.socket subclass rather than a wrapper thereof. (Also, the pseudo _fileobject was replaced with an appropriate, real, io class.)
msg199134 - (view) Author: Florent Viard (fviard) Date: 2013-10-07 09:56
Hi Terry,
I think you misunderstood what i was trying to say.
Maybe fileno should raise a ValueError (and not -1) in that case. That is the question. It should only be able to be something understood by "select.select".
But currently it is an Bug/Crash as the case where self.fp is None is not handled before trying to get self.fp.fileno() and so this raise an AttributeError and not a ValueError. And so, it is certainly not managed by select.

Please really understand that here I speak about the "def fileno()" function that is inside Lib/http/client.py" and not about the "def fileno()" that is in socket.py.
msg199159 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-10-07 20:13
We are both talking about 2.7 httplib.HTTPResponse.fileno. I should have said that users should try: ...fileno(); except AttributeError rather than ValueError. Any caller can understand AttributeError as well as ValueError or any other exception.

It is not an error for a python function to raise an exception. In fact, it is the normal thing to do when it cannot do as requested. API changes are not allowed in 2.7 and this one would be dubious even in a future 3.x.

I do think the doc should be more informative and that

Return(s) the fileno of the underlying socket.

should be extended to indicate the specific exception raised when that is not possible.

Return the fileno of the underlying socket if there is one or raise AttributeError.

This should also be added as a docstring (currently missing).

The above is also true for 3.x as self.fp continues to be replaced with None on closing.

The HTTPResponse docs are known to be deficient. #3430

--
I do not think select needs to be changed to understand the HTTPResponse.fileno error indicator because it does not call that method. As indicated in the traceback, it calls (in 2.7) socket._fileinput.fileno. I believe it is only a bug in nappstore (that would be harder to reproduce in 3.x) that _fileinput.fileno is forwarded to HTTPResponse.fileno instead of _socket.fileno. The latter seem to be the clear intention. Even if I am wrong, changing something in the select or socket modules would be a different issue from changing something in the client module.
msg199198 - (view) Author: Florent Viard (fviard) Date: 2013-10-08 12:30
Thank you for your reply.
But I just realised that in my bug issue, I completely forgot to indicate what is "req" and so this is maybe the root of you telling me that the best is to fix the client code side as the traceback could be confusing.

This is how is defined req.:
opener = urllib2.build_opener(proxy_handler)
req    = urllib2.Request(url_src)

then:
while 1:
  read_list = select([req], ...)[0]
  if read_list:
    req.read(CHUNK_SIZE)


I found this issue with python 2.7, but I don't care a lot for it to be fixed in 2.7. As I saw that the code looks unchanged in python 3.x, I just reported the issue for it to be fixed/better handled in 3.x :-)
msg199211 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-10-08 15:51
It seems to me that there is indeed an issue of some sort here, but its locus is (to me) unclear.  I haven't commented before this because I wanted to read the docs...but I haven't had time yet :)  

One question is, is it even expected that passing a Request to select will work?  If it *is* expected, then what is the API that req should be conforming to?  This API may be an implicit one that is not documented, or perhaps it is documented in select (I haven't checked).  If req is conforming to the explicit or implicit API, then the bug would be in select.  Otherwise it is in httplib.

Or, if this isn't something we've been supporting in the past, then as Terry says it is a new feature.
msg199212 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-10-08 15:53
s/httplib/urllib/
msg199221 - (view) Author: Florent Viard (fviard) Date: 2013-10-08 17:17
R. David, what you say is correct, supporting "select" that would be nice but i'm also not sure that is supposed to, and in that case, maybe select as to be fixed for that.

But:
a) As urllib2 through httplib provide publicly a fileno, i was excepting so.
b) The real point of my issue is that i noticed that there is 3 different return values, for the similar fileno function in 3 different modules of python, when the file descriptor is closed or inexistant:
- in urllib2->httplib: AttributeError as "None" as no "fileno()" attribute.
- in socket : ValueError
- in fileinput: -1

And for the 2 firsts, one is finally using the fileno function of the other.
msg199245 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-10-08 20:24
OK, I've looked at the docs and code, and as far as I can see this bug does not exist in Python3.  Or at least in 3.4, which is the only place I'd feel safe about making a change to the exception type.

To summarize: in 3.4 socket logic is based on RawIOBase, as is HTTPResponse.  And RawIOBase checks to see if the file is closed and raises a ValueError if it is, when fileno is called on the 'fp' attribute of the HTTPResponse.  At least, that's what I think based on reading the code.

So, unless you can reproduce the error in 3.3 and/or 3.4, I think we should close this issue as out of date, since as Terry said there is a non-trivial danger of backward incompatibility if we were to change the exception type in 2.7.
msg199246 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-10-08 20:36
Florent, for future reference, marking an issue for 2.7 says to us "I  want this fixed for 2.7".

I agree that having 3 different error indicators for 3 similar functions is nasty. But this is partly due to the difference between object that *has* a fd (socket) versus a object that *wraps such an object. The problem is that changing any of them could break code that just uses one of them. 

The select doc says that a 'waitable' object must have .fileno() that return a valid fd int. It says nothing about an allowed error return. This could be interpreted as meaning that not returning an fd int makes the object (such as a closed HTTPResponse) unwaitable. Or this could be interpreted as a deficiency in the select doc or code. In the module/selectmodule.c code, the various poll methods interpret -1 as the error return for a closed fd. I do not know how it deals with socket.fileno raising ValueError (or returning None?).

In the client doc, none of several examples of using HTTPResponses use select. They just use the various methods to retrieve data. I could interpret this to mean that they are not intended to work with select. The fact that they do not, reinforces that.
msg199252 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-08 21:11
I think ValueError should generally be raised when calling fileno() on a closed file. However, this is not something we're gonna change in a bugfix release, so it would go in 3.4 at the earliest.
History
Date User Action Args
2013-11-05 02:54:49terry.reedysetversions: - Python 2.7, Python 3.3
2013-10-08 21:11:00pitrousetnosy: + pitrou
messages: + msg199252
2013-10-08 20:36:40terry.reedysetmessages: + msg199246
2013-10-08 20:24:19r.david.murraysetmessages: + msg199245
2013-10-08 17:17:34fviardsetmessages: + msg199221
2013-10-08 15:53:30r.david.murraysetmessages: + msg199212
2013-10-08 15:51:59r.david.murraysetmessages: + msg199211
2013-10-08 12:30:42fviardsetmessages: + msg199198
2013-10-07 20:13:31terry.reedysetassignee: docs@python
type: behavior -> enhancement
components: + Documentation, - Library (Lib)
versions: + Python 3.3, Python 3.4
keywords: + patch, easy
nosy: + docs@python

messages: + msg199159
stage: test needed -> needs patch
2013-10-07 20:13:19terry.reedylinkissue3430 dependencies
2013-10-07 09:56:55fviardsetmessages: + msg199134
2013-10-05 02:55:14terry.reedysetnosy: + terry.reedy

messages: + msg198979
versions: - Python 3.3, Python 3.4, Python 3.5
2013-10-05 00:40:07ezio.melottisetstage: test needed
type: crash -> behavior
versions: - Python 2.6, Python 3.1, Python 3.2
2013-10-03 18:07:51r.david.murraysetnosy: + r.david.murray
2013-10-03 17:12:38fviardcreate