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

asyncore.dispatcher's __getattr__ method produces confusing effects #52729

Closed
djc opened this issue Apr 21, 2010 · 13 comments
Closed

asyncore.dispatcher's __getattr__ method produces confusing effects #52729

djc opened this issue Apr 21, 2010 · 13 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@djc
Copy link
Member

djc commented Apr 21, 2010

BPO 8483
Nosy @josiahcarlson, @giampaolo, @djc, @bitdancer
Files
  • asyncore.patch
  • 2.7.patch
  • 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 = 'https://github.com/giampaolo'
    closed_at = <Date 2010-05-06.18:57:00.702>
    created_at = <Date 2010-04-21.10:55:24.049>
    labels = ['type-bug', 'library']
    title = "asyncore.dispatcher's __getattr__ method produces confusing effects"
    updated_at = <Date 2010-05-06.18:57:00.699>
    user = 'https://github.com/djc'

    bugs.python.org fields:

    activity = <Date 2010-05-06.18:57:00.699>
    actor = 'giampaolo.rodola'
    assignee = 'giampaolo.rodola'
    closed = True
    closed_date = <Date 2010-05-06.18:57:00.702>
    closer = 'giampaolo.rodola'
    components = ['Library (Lib)']
    creation = <Date 2010-04-21.10:55:24.049>
    creator = 'djc'
    dependencies = []
    files = ['17026', '17205']
    hgrepos = []
    issue_num = 8483
    keywords = ['patch']
    message_count = 13.0
    messages = ['103816', '103821', '103834', '103836', '103857', '103861', '103866', '103870', '103873', '103879', '103941', '104974', '105158']
    nosy_count = 5.0
    nosy_names = ['josiahcarlson', 'giampaolo.rodola', 'josiah.carlson', 'djc', 'r.david.murray']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue8483'
    versions = ['Python 2.6', 'Python 3.1', 'Python 2.7', 'Python 3.2']

    @djc
    Copy link
    Member Author

    djc commented Apr 21, 2010

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

    @djc djc added the stdlib Python modules in the Lib dir label Apr 21, 2010
    @djc
    Copy link
    Member Author

    djc commented Apr 21, 2010

    Current guess for this behavior: dispatcher doesn't have __str__, so __getattr__ escalates to _socket.socket.__str__, which gets redirected to _socket.socket.__repr__.

    @bitdancer
    Copy link
    Member

    I'm not clear on what you are finding weird, here. Your issue title talks about __repr__, but your last post talks about __str__.

    @djc
    Copy link
    Member Author

    djc commented Apr 21, 2010

    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.

    @giampaolo
    Copy link
    Contributor

    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'
    >>>

    @bitdancer
    Copy link
    Member

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

    @giampaolo
    Copy link
    Contributor

    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.

    @bitdancer
    Copy link
    Member

    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.

    @bitdancer bitdancer changed the title asyncore.dispatcher.__repr__() is weird asyncore.dispatcher's __getattr__ method produces confusing effects Apr 21, 2010
    @bitdancer
    Copy link
    Member

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

    @giampaolo
    Copy link
    Contributor

    You're absolutely right.
    Patch in attachment.

    @djc
    Copy link
    Member Author

    djc commented Apr 22, 2010

    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.

    @giampaolo
    Copy link
    Contributor

    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.

    @giampaolo giampaolo self-assigned this May 4, 2010
    @giampaolo giampaolo added the type-bug An unexpected behavior, bug, or error label May 4, 2010
    @giampaolo
    Copy link
    Contributor

    Fixed in r80875 (2.7), r80876 (3.2), r80878 (2.6) and r80879 (3.1) which also includes changes to fix bpo-8573.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants