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

Instance methods and WeakRefs don't mix. #58836

Closed
Sundance mannequin opened this issue Apr 20, 2012 · 12 comments
Closed

Instance methods and WeakRefs don't mix. #58836

Sundance mannequin opened this issue Apr 20, 2012 · 12 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@Sundance
Copy link
Mannequin

Sundance mannequin commented Apr 20, 2012

BPO 14631
Nosy @jcea, @mdickinson, @pitrou, @durban
Files
  • WeakCallableRef.py
  • weakmethod.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 = None
    closed_at = <Date 2012-11-17.18:01:46.023>
    created_at = <Date 2012-04-20.13:49:42.022>
    labels = ['type-feature', 'library']
    title = "Instance methods and WeakRefs don't mix."
    updated_at = <Date 2012-11-26.17:46:13.363>
    user = 'https://bugs.python.org/Sundance'

    bugs.python.org fields:

    activity = <Date 2012-11-26.17:46:13.363>
    actor = 'jcea'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-11-17.18:01:46.023>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2012-04-20.13:49:42.022>
    creator = 'Sundance'
    dependencies = []
    files = ['25288', '27960']
    hgrepos = []
    issue_num = 14631
    keywords = ['patch']
    message_count = 12.0
    messages = ['158828', '158834', '175343', '175344', '175372', '175384', '175391', '175392', '175474', '175475', '175792', '175793']
    nosy_count = 7.0
    nosy_names = ['jcea', 'mark.dickinson', 'pitrou', 'daniel.urban', 'python-dev', 'Sundance', 'sfeltman']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue14631'
    versions = ['Python 3.4']

    @Sundance
    Copy link
    Mannequin Author

    Sundance mannequin commented Apr 20, 2012

    SUMMARY
    =======

    The behavior of instance methods makes it impossible to keep a weakref on them. The reference dies instantly even if the instance and the method still exist.

    EXAMPLE
    =======

    >> import weakref

    >> callbacks = weakref.WeakSet()

    >>> def callback1():
    ...   print "In callback1."
    
    >>> class SomeClass:
    ...   def callback2(self):
    ...     print "In callback2."

    >> some_instance = SomeClass()

    >> callbacks.add( callback1 )
    >> callbacks.add( some_instance.callback2 )

    >>> for callback in callbacks:
    ...   callback()
    In callback1.

    >> ## callback2 is never called!

    ANALYSIS
    ========

    The WeakSet in the example above, and the weakref.ref() it employs, actually behave as specified. It's the particular nature of bound methods that causes the unexpected behavior.

    From what I understand, instance methods are bound dynamically when looked up on the instance. A new object of type instancemethod is created each time:

    >>> t1 = some_instance.callback
    >>> t2 = some_instance.callback
    >>> t1 is t2
    False

    So when a program calls weakref.ref(some_instance.callback), a new instancemethod object is created on the fly, a weakref to that object is created... and the instancemethod object dies, because its refcount is 0.

    This fundamental difference between instance methods and other callables makes it painful to implement weakly referencing callback registries.

    PROPOSED SOLUTION
    =================

    Changing the fundamental nature of instance methods to accommodate one single corner case doesn't seem worthwhile.

    Similarly, changing the behavior of weakref.ref(), which does work as specified, is probably not a good idea.

    My approach is thus to provide a new helper, WeakCallableRef, that behaves like weakref.ref(), but is safe to use on callables and does what you would naturally expect with instance methods.

    It works by binding the lifetime of the ref to that of 1/ the instance bearing the method, and 2/ the unbound method itself. It is also safe to use on other function types beside instance methods, so implementations of callback registries don't have to special case depending on the callable type.

    The unexpected behavior should also be mentioned somewhere in the weakref documentation, by the way.

    See attached file for a proposed implementation.

    @Sundance Sundance mannequin added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels Apr 20, 2012
    @mdickinson
    Copy link
    Member

    I quite like the WeakCallableRef idea, having had to work around this problem myself in the past (using a similar mechanism).

    This looks like a feature request rather than a bug report, though; changing Type and Versions accordingly.

    @mdickinson mdickinson added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Apr 20, 2012
    @mdickinson
    Copy link
    Member

    @sfeltman
    Copy link
    Mannequin

    sfeltman mannequin commented Nov 11, 2012

    Just a note this is also referred to as a "WeakMethod" by some folks (so this ticket shows up in those searches). See also:
    http://bugs.python.org/issue813299
    http://bugs.python.org/issue7464
    http://bugs.python.org/issue16452

    @pitrou
    Copy link
    Member

    pitrou commented Nov 11, 2012

    WeakMethod sounds like a good name to me. However, instead of acting as a proxy, I think it should act as a regular weakref (i.e. you have to call it to get the actual method object).

    @pitrou
    Copy link
    Member

    pitrou commented Nov 11, 2012

    Here is a patch adding a WeakMethod class. It still lacks docs, I'll add some if people agree on the principle.

    @sfeltman
    Copy link
    Mannequin

    sfeltman mannequin commented Nov 11, 2012

    The WeakCallableRef that was attached seemed to support regular functions (or anything callable) which is nice. The naming also leaves room for a WeakCallableProxy.

    @sfeltman
    Copy link
    Mannequin

    sfeltman mannequin commented Nov 11, 2012

    Some more complex examples from various libraries:

    https://github.com/django/django/blob/master/django/dispatch/saferef.py
    https://github.com/11craft/louie/blob/master/louie/saferef.py

    I think both of these originated in pydispatcher.

    @mdickinson
    Copy link
    Member

    The patch looks okay to me.

    What does inheriting from 'ref' buy you? This feels a bit strange to me: the way I think of it, the WeakMethod *has* a weakref to the underlying object, rather than *being* a weakref to the underlying object. The __repr__ also seems a bit misleading as a result:

    >>> o = Object()
    >>> m = o.some_method
    >>> WeakMethod(m)
    <weakref at 0x100665ae0; to 'Object' at 0x101115840>

    @pitrou
    Copy link
    Member

    pitrou commented Nov 12, 2012

    What does inheriting from 'ref' buy you?

    Hmm, I'm not sure. I thought I'd mimick KeyedRef's inheritance design, plus isinstance(..., weakref.ref) works, and composition would make the object slightly bigger. Other than that, probably nothing.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 17, 2012

    New changeset 27c20650aeab by Antoine Pitrou in branch 'default':
    Issue bpo-14631: Add a new :class:`weakref.WeakMethod` to simulate weak references to bound methods.
    http://hg.python.org/cpython/rev/27c20650aeab

    @pitrou
    Copy link
    Member

    pitrou commented Nov 17, 2012

    Committed, thank you!

    @pitrou pitrou closed this as completed Nov 17, 2012
    @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-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants