Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AttributeError: 'NoneType' in http/client.py when using select when file descriptor is closed. #63353

Closed
fviard mannequin opened this issue Oct 3, 2013 · 12 comments
Closed
Labels
docs Documentation in the Doc dir easy type-feature A feature request or enhancement

Comments

@fviard
Copy link
Mannequin

fviard mannequin commented Oct 3, 2013

BPO 19154
Nosy @terryjreedy, @pitrou, @bitdancer, @fviard

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-10-03.17:12:38.898>
labels = ['easy', 'type-feature', 'docs']
title = "AttributeError: 'NoneType' in http/client.py when using select when file descriptor is closed."
updated_at = <Date 2013-11-05.02:54:49.567>
user = 'https://github.com/fviard'

bugs.python.org fields:

activity = <Date 2013-11-05.02:54:49.567>
actor = 'terry.reedy'
assignee = 'docs@python'
closed = False
closed_date = None
closer = None
components = ['Documentation']
creation = <Date 2013-10-03.17:12:38.898>
creator = 'fviard'
dependencies = []
files = []
hgrepos = []
issue_num = 19154
keywords = ['patch', 'easy']
message_count = 11.0
messages = ['198902', '198979', '199134', '199159', '199198', '199211', '199212', '199221', '199245', '199246', '199252']
nosy_count = 5.0
nosy_names = ['terry.reedy', 'pitrou', 'r.david.murray', 'docs@python', 'fviard']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19154'
versions = ['Python 3.4']

@fviard
Copy link
Mannequin Author

fviard mannequin commented Oct 3, 2013

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

@fviard fviard mannequin added type-crash A hard crash of the interpreter, possibly with a core dump stdlib Python modules in the Lib dir labels Oct 3, 2013
@ezio-melotti ezio-melotti added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 5, 2013
@terryjreedy
Copy link
Member

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.)

@fviard
Copy link
Mannequin Author

fviard mannequin commented Oct 7, 2013

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.

@terryjreedy
Copy link
Member

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. bpo-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.

@terryjreedy terryjreedy added docs Documentation in the Doc dir easy and removed stdlib Python modules in the Lib dir labels Oct 7, 2013
@terryjreedy terryjreedy added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Oct 7, 2013
@fviard
Copy link
Mannequin Author

fviard mannequin commented Oct 8, 2013

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 :-)

@bitdancer
Copy link
Member

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.

@bitdancer
Copy link
Member

s/httplib/urllib/

@fviard
Copy link
Mannequin Author

fviard mannequin commented Oct 8, 2013

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.

@bitdancer
Copy link
Member

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.

@terryjreedy
Copy link
Member

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.

@pitrou
Copy link
Member

pitrou commented Oct 8, 2013

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.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@mblahay
Copy link
Contributor

mblahay commented Apr 24, 2023

@terryjreedy@pitrou@bitdancer@fviard @ambv

This issue is 10 years old and appears to only affect 2.7, which is no longer supported. This issue should be closed in order to clean up the issue list.

@ambv ambv closed this as completed Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir easy type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

6 participants