classification
Title: asyncore.dispatcher's __getattr__ method produces confusing effects
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: djc, giampaolo.rodola, josiah.carlson, josiahcarlson, r.david.murray
Priority: normal Keywords: patch

Created on 2010-04-21 10:55 by djc, last changed 2010-05-06 18:57 by giampaolo.rodola. This issue is now closed.

Files
File name Uploaded Description Edit
asyncore.patch giampaolo.rodola, 2010-04-21 18:24
2.7.patch giampaolo.rodola, 2010-05-04 18:58
Messages (13)
msg103816 - (view) Author: Dirkjan Ochtman (djc) * (Python committer) Date: 2010-04-21 10:55
This is rather confusing:

Python 2.6.4 (r264:75706, Mar  8 2010, 08:41:55)
[GCC 4.3.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket, asyncore
>>> class T:
...     pass
...
>>> t = T()
>>> print t
<__main__.T instance at 0x18237a0>
>>> print repr(t)
<__main__.T instance at 0x18237a0>
>>> class A(asyncore.dispatcher):
...     pass
...
>>> a = A()
>>> print a
None
>>> print repr(a)
<__main__.A at 0x179c0e0>
>>> class B(asyncore.dispatcher):
...     def __init__(self, *args):
...             asyncore.dispatcher.__init__(self, *args)
...
>>> sock = socket.socket()
>>> b = B(sock)
>>> print b
<socket._socketobject object at 0x7fcef226e8a0>
>>> print repr(b)
<__main__.B at 0x179c0e0>

Here's dispatcher's __repr__:

    def __repr__(self):
        status = [self.__class__.__module__+"."+self.__class__.__name__]
        if self.accepting and self.addr:
            status.append('listening')
        elif self.connected:
            status.append('connected')
        if self.addr is not None:
            try:
                status.append('%s:%d' % self.addr)
            except TypeError:
                status.append(repr(self.addr))
        return '<%s at %#x>' % (' '.join(status), id(self))

Which doesn't seem excessively complex...
msg103821 - (view) Author: Dirkjan Ochtman (djc) * (Python committer) Date: 2010-04-21 11:33
Current guess for this behavior: dispatcher doesn't have __str__, so __getattr__ escalates to _socket.socket.__str__, which gets redirected to _socket.socket.__repr__.
msg103834 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-21 12:40
I'm not clear on what you are finding weird, here.  Your issue title talks about __repr__, but your last post talks about __str__.
msg103836 - (view) Author: Dirkjan Ochtman (djc) * (Python committer) Date: 2010-04-21 12:56
David, just have a look at the interpreter transcript, in particular the results of the print statements. I'm not sure why it happens, in my previous message I just stated a hypothesis.
msg103857 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-04-21 15:31
I think the problem relies in here:

    # cheap inheritance, used to pass all other attribute
    # references to the underlying socket object.
    def __getattr__(self, attr):
        return getattr(self.socket, attr)

I wonder why this has been added in the first place.
It also causes confusing error messages when accessing undefined attributes:

>>> class B(asyncore.dispatcher):
...     def __init__(self, *args):
...             asyncore.dispatcher.__init__(self, *args)
...
>>> b = B()
>>> b.blabla
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/asyncore.py", line 394, in __getattr__
    return getattr(self.socket, attr)
AttributeError: '_socketobject' object has no attribute 'blabla'
>>>
msg103861 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-21 16:20
I somehow missed the fact that B was a dispatcher subclass.  Too early in my morning, I guess.

So, I think the title of this issue should be "asyncore.dispatcher's __repr__ not being used as fallback for __str__", and your speculation looks likely to be on target, as Giampaolo says.

Unfortunately, even if forwarding everything to the socket is questionable, it would probably be a really bad idea to change it, since there is likely code in the field depending on this behavior.

The specific case of __str__ could be fixed by adding an __str__ to dispatcher, and that would be a lot less likely to break anything other than perhaps the odd doctest here and there (but that would mean it still shouldn't be changed in 2.6).
msg103866 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-04-21 17:34
> Unfortunately, even if forwarding everything to the socket is 
> questionable, it would probably be a really bad idea to change it, 
> since there is likely code in the field depending on this behavior.

I agree, even if it would break a code which is fundamentally *already* wrong. Maybe we can do this in 2.7 and 3.2 only?
The AttributeError message is *very* confusing.
Actually I've seen it a lot of times through the years and never figured out it had to do with that weird __getattr__ statement.
msg103870 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-21 17:51
Well, the attribute error message could be fixed, too, by enclosing the forwarding getattr in a try/except and generating the "correct" attribute error text.

Changing the forwarding behavior itself is the kind of issue that would need to be discussed on python-dev, and we'd need to make sure that the various asyncore constiuencies got a chance to chime in (remember the previous breakage of asyncore monkey-patching in 2.6 and the rancor it caused).  My guess is that we can't change it, though.  Certainly not in 2.7.

I'm changing the title of the issue to more accurately reflect the discussion.
msg103873 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-21 17:57
By the way, the __getattr_ was in the very first version of the code checked in to the repository.  I think the chances of removing it are pretty much zero :)
msg103879 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-04-21 18:24
You're absolutely right.
Patch in attachment.
msg103941 - (view) Author: Dirkjan Ochtman (djc) * (Python committer) Date: 2010-04-22 08:24
I definitely like the patch, but ideally we would also have a patch adding a __str__ on dispatcher, right? Which would just return the repr(self) result.
msg104974 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-04 18:58
As per discussion on #python-dev we think it's better to proceed as follows:

for python 2.7 and 3.2: 
 - fix __getattr__ error message
 - raise a DeprecationWarning if cheap inheritance is used and definitively remove its support in the next major version
 - create an alias for __str__

For Python 2.6 and 3.1: just fix __getattr__ error message.

Patch for Python 2.7 is in attachment.
msg105158 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-06 18:57
Fixed in r80875 (2.7), r80876 (3.2), r80878 (2.6) and r80879 (3.1) which also includes changes to fix issue 8573.
History
Date User Action Args
2010-05-06 18:57:00giampaolo.rodolasetstatus: open -> closed
resolution: fixed
messages: + msg105158

stage: commit review -> resolved
2010-05-04 18:58:04giampaolo.rodolasetfiles: + 2.7.patch
versions: + Python 3.1, Python 2.7, Python 3.2
messages: + msg104974

assignee: giampaolo.rodola
type: behavior
stage: commit review
2010-04-22 08:24:24djcsetmessages: + msg103941
2010-04-21 18:24:51giampaolo.rodolasetfiles: + asyncore.patch
keywords: + patch
messages: + msg103879
2010-04-21 17:57:16r.david.murraysetmessages: + msg103873
2010-04-21 17:51:24r.david.murraysetpriority: low -> normal

messages: + msg103870
title: asyncore.dispatcher.__repr__() is weird -> asyncore.dispatcher's __getattr__ method produces confusing effects
2010-04-21 17:34:30giampaolo.rodolasetmessages: + msg103866
2010-04-21 16:20:31r.david.murraysetmessages: + msg103861
2010-04-21 15:31:03giampaolo.rodolasetmessages: + msg103857
2010-04-21 12:56:20djcsetmessages: + msg103836
2010-04-21 12:40:27r.david.murraysetpriority: low
nosy: + r.david.murray
messages: + msg103834

2010-04-21 12:13:27giampaolo.rodolasetnosy: + giampaolo.rodola
2010-04-21 11:33:55djcsetmessages: + msg103821
2010-04-21 11:02:50djcsetnosy: + josiahcarlson, josiah.carlson
2010-04-21 10:55:24djccreate