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

Better support for finalization with weakrefs #59733

Closed
sbt mannequin opened this issue Aug 1, 2012 · 28 comments
Closed

Better support for finalization with weakrefs #59733

sbt mannequin opened this issue Aug 1, 2012 · 28 comments
Labels
type-feature A feature request or enhancement

Comments

@sbt
Copy link
Mannequin

sbt mannequin commented Aug 1, 2012

BPO 15528
Nosy @jcea, @pitrou, @vstinner, @asvetlov, @ambv, @serhiy-storchaka
Files
  • finalize.patch
  • finalize.patch
  • finalize.patch
  • finalize.patch
  • finalize.patch
  • finalize.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 2013-06-08.16:54:33.024>
    created_at = <Date 2012-08-01.16:18:46.340>
    labels = ['type-feature']
    title = 'Better support for finalization with weakrefs'
    updated_at = <Date 2013-06-08.16:54:33.023>
    user = 'https://bugs.python.org/sbt'

    bugs.python.org fields:

    activity = <Date 2013-06-08.16:54:33.023>
    actor = 'sbt'
    assignee = 'sbt'
    closed = True
    closed_date = <Date 2013-06-08.16:54:33.024>
    closer = 'sbt'
    components = []
    creation = <Date 2012-08-01.16:18:46.340>
    creator = 'sbt'
    dependencies = []
    files = ['26649', '26701', '26708', '26879', '26900', '30140']
    hgrepos = []
    issue_num = 15528
    keywords = ['patch']
    message_count = 28.0
    messages = ['167142', '167419', '167431', '167437', '167458', '167503', '167507', '167556', '167952', '167998', '168442', '168592', '172077', '182083', '182092', '182100', '182104', '182123', '182124', '188455', '188457', '188459', '188465', '188466', '188474', '190798', '190802', '190809']
    nosy_count = 8.0
    nosy_names = ['jcea', 'pitrou', 'vstinner', 'asvetlov', 'lukasz.langa', 'python-dev', 'sbt', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue15528'
    versions = ['Python 3.4']

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 1, 2012

    A patch with docs and tests for the idea I suggested on python-ideas:

    http://comments.gmane.org/gmane.comp.python.ideas/15863
    

    To repeat what I wrote there, the current issues with weakref callbacks include:

    1. They are rather low level, and working out how to use them
      correctly requires a bit of head scratching. One must find
      somewhere to store the weakref till after the referent is dead, and
      without accidentally keeping the referent alive. Then one must
      ensure that the callback frees the weakref (without leaving any
      remnant ref-cycles).

      When it is an option, using a __del__ method is far less hassle.

    2. Callbacks invoked during interpreter shutdown are troublesome. For
      instance they can throw exceptions because module globals have been
      replaced by None.

    3. Sometimes you want the callback to be called at program exit even
      if the referent is still alive.

    4. Exceptions thrown in callbacks do not produce a traceback. This
      can make debugging rather awkward. (Maybe this is because printing
      tracebacks is problematic during interpreter shutdown?)

    (Note that problems 2-4 also apply to using __del__ methods.)

    Trivial usage of the class looks like

        >>> class Kenny: pass
        ...
        >>> kenny = Kenny()
        >>> finalize(kenny, print, "you killed kenny!")
        <finalize.finalize object at 0xffed4f2c>
        >>> del kenny
        you killed kenny!

    @sbt sbt mannequin added the type-feature A feature request or enhancement label Aug 1, 2012
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 4, 2012

    I don't quite understand the purpose of your suggestions. What can you do with it help, what you can not do with contextlib.ExitStack, atexit, __del__ method, weakref.WeakKeyDictionary or weakref.ref? I read the documentation, but the meaning eludes me.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 4, 2012

    I don't quite understand the purpose of your suggestions. What can you do
    with it help, what you can not do with contextlib.ExitStack, atexit,
    __del__ method, weakref.WeakKeyDictionary or weakref.ref? I read the
    documentation, but the meaning eludes me.

    finalize does not "compete" with contextlib.ExitStack, atexit and
    weakref.WeakKeyDictionary. It only competes with __del__ and weakref
    callbacks.

    Points 1 and 2 in my first message are the main points. Also, read the
    warning at

    http://docs.python.org/py3k/reference/datamodel.html#object.\_\_del__

    which also applies to weakref callbacks.

    Other problems with __del__:

    • Ref cycles which contain an object with a __del__ method are immortal

    • __del__ methods can "ressurect" the object.

    There was actually a proposal to remove or replace __del__ methods in
    Python 3000. See the "Removing __del__" thread(s):

    http://mail.python.org/pipermail/python-3000/2006-September/thread.html#3797

    As for weakref callbacks, I think they are just too difficult to use correctly
    unless you are very familiar with them.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 4, 2012

    finalize does not "compete" with contextlib.ExitStack, atexit and
    weakref.WeakKeyDictionary. It only competes with __del__ and weakref
    callbacks.

    What kind of problems you solve with __del__ and weakref callbacks? For most
    of them contextlib.ExitStack and atexit are safer and better fit.

    Points 1 and 2 in my first message are the main points. Also, read the
    warning at

    For point 1: global weakref.WeakKeyDictionary is good store for weak refs with
    callbacks.

    global weakdict
    weakdict[kenny] = weakref.ref(kenny, lambda _: print("you killed kenny!"))

    If you do not want to work at a low level, in most cases fit the above-
    mentioned high-level tools.

    For point 2 Antoine has noted that it is a known issue and there is a solution
    (bpo-812369).

    http://docs.python.org/py3k/reference/datamodel.html#object.\_\_del__

    which also applies to weakref callbacks.

    Is this your point 2?

    Other problems with __del__:

    • Ref cycles which contain an object with a __del__ method are immortal

    It is use case for weakrefs which break reference loops.

    • __del__ methods can "ressurect" the object.

    What you are meaning?

    Relying on GC is dangerous thing for resource releasing. And it even more
    dangerous for alternative implementations without reference counting. That is
    why in recent times in Python there has been a tendency to RAII strategy
    (context managers).

    Can you give examples, where the use of finalize necessary or more convenient
    the other ways?

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 4, 2012

    For point 1: global weakref.WeakKeyDictionary is good store for weak refs with
    callbacks.

    global weakdict
    weakdict[kenny] = weakref.ref(kenny, lambda _: print("you killed kenny!"))

    That depends on kenny being hashable.

    It also surprises me a bit that it works. It seems to depend on unguaranteed
    implementation details: PyObject_ClearWeakRefs() copies all weakrefs with
    callbacks to a tuple before invoking callbacks. If this were not the case
    then I think the weakref you created (which is scheduled to fire second) would
    be garbage collected before its callback could be invoked.

    I think users should have a documented way to schedule callbacks without having to
    explicitly save the weakref.

    For point 2 Antoine has noted that it is a known issue and there is a solution
    (bpo-812369).

    That has been languishing for 9 years. I would not hold your breath.

    > http://docs.python.org/py3k/reference/datamodel.html#object.\_\_del__
    >
    > which also applies to weakref callbacks.

    Is this your point 2?

    Yes.

    Relying on GC is dangerous thing for resource releasing. And it even more
    dangerous for alternative implementations without reference counting. That is
    why in recent times in Python there has been a tendency to RAII strategy
    (context managers).

    I agree that explicit clean up using context managers to be strongly encouraged.
    But resources should still be released if the object is garbage collected. Or
    do you think that file objects should stop bothering to close their fds when they
    are deallocated?

    Can you give examples, where the use of finalize necessary or more convenient
    the other ways?

    multiprocessing (which I orginally wrote) makes extensive use of finalizers,
    (although with a different implementation and api). In some cases that is because
    at the time I wanted to support versions of Python which did not have the with
    statement.

    But there are various things there that depend on finalizers, and on being able
    to guarantee that they get called on exit (in a predictable order).

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 5, 2012

    Updated patch.

    get() and put() replaced by peek() and detach(). Added isalive(). Now finalizer class has no state, i.e. __slots__ == ().

    @pitrou
    Copy link
    Member

    pitrou commented Aug 5, 2012

    I don't understand why you have two names: finalize and Finalizer. Let's keep only one of them.

    isalive() could be a read-only property named "alive" instead.

    In _make_callback, I still think the default error reporting mechanism should be kept. It can be improved separately.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 6, 2012

    In _make_callback, I still think the default error reporting mechanism
    should be kept. It can be improved separately.

    New patch. This time I have got rid of _make_callback, and just given __call__ an ignored optional argument.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 11, 2012

    In the latest patch, what are "broken" and "priority" for?

    Also, I would question why atexit is false by default. I would find it personally less surprising to be true.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 11, 2012

    In the latest patch, what are "broken" and "priority" for?

    They are for a subclass used by multiprocessing. But it is premature to worry about subclassing, so I will take them out.

    Also, I would question why atexit is false by default. I would find it
    personally less surprising to be true.

    OK.

    One thing I do worry about is having the loop in the exit function to run any finalizers created during the exit function. The current implementation will run these extra finalizers at the wrong time. Fixing this could probably be done by using a dirty flag and disabling gc while running the finalizers.

    I wonder if it would be better to not call finalizers created during the exit function. We cannot guarantee that every finalizer created during shutdown is run, so is a best effort attempt really worth the effort?

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 17, 2012

    Updated patch.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Aug 19, 2012

    New patch.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 5, 2012

    In:

    + except:
    + sys.excepthook(*sys.exc_info())

    I would write "except Exception" instead. You don't want to trap e.g. KeyboardInterrupt.

    For clarity, I would also add "_dirty = False" at the finalize top-level.
    Otherwise, looks fine to me.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 14, 2013

    Richard, do you still want to push this forward? Otherwise I'd like to finalize the patch (in the other sense ;-).

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Feb 14, 2013

    Richard, do you still want to push this forward? Otherwise I'd like to
    finalize the patch (in the other sense ;-).

    I started to worry a bit about daemon threads. I think they can still run while atexit functions are being run.* So if a daemon thread creates an atexit finalizer during shutdown it may never be run.

    I am not sure how much to worry about this potential race. Maybe a lock could be used to cause any daemon threads which try to create finalizers to block.

    • Is it necessary/desirable to allow daemon threads to run during shutdown. Maybe blocking thread switching at shutdown could cause deadlocks?

    @pitrou
    Copy link
    Member

    pitrou commented Feb 14, 2013

    Is it necessary/desirable to allow daemon threads to run during
    shutdown. Maybe blocking thread switching at shutdown could cause
    deadlocks?

    Mmmh... thread switching is already blocked at shutdown:
    http://hg.python.org/cpython/file/0f827775f7b7/Python/ceval.c#l434

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Feb 14, 2013

    On 14/02/2013 3:16pm, Antoine Pitrou wrote:

    Mmmh... thread switching is already blocked at shutdown:
    http://hg.python.org/cpython/file/0f827775f7b7/Python/ceval.c#l434

    But in Py_Finalize(), call_py_exitfuncs() is called *before* _Py_Finalizing is set to a non-NULL value.

    http://hg.python.org/cpython/file/0f827775f7b7/Python/pythonrun.c#l492
    

    @pitrou
    Copy link
    Member

    pitrou commented Feb 14, 2013

    But in Py_Finalize(), call_py_exitfuncs() is called *before*
    _Py_Finalizing is set to a non-NULL value.

    http://hg.python.org/cpython/file/0f827775f7b7/Python/pythonrun.c#l492

    Ah, right. But is it any different from, e.g., registering an atexit
    handler from a daemon thread?
    In any case, I think it's just something we'll have to live with. Daemon
    threads are not a terrific idea in general.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Feb 14, 2013

    In any case, I think it's just something we'll have to live with. Daemon
    threads are not a terrific idea in general.

    I agree.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented May 5, 2013

    Here is an updated patch. It is only really the example in the docs which is different, plus a note about daemon threads.

    Antoine, do think this is ready to be committed?

    @pitrou
    Copy link
    Member

    pitrou commented May 5, 2013

    Well, assuming there are no significant changes in the code (I haven't checked), +1 for committing.

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented May 5, 2013

    The only (non-doc, non-comment) changes were the two one-liners you suggested in msg172077. So I will commit.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 5, 2013

    New changeset 2e446e87ac5b by Richard Oudkerk in branch 'default':
    Issue bpo-15528: Add weakref.finalize to support finalization using
    http://hg.python.org/cpython/rev/2e446e87ac5b

    @vstinner
    Copy link
    Member

    vstinner commented May 5, 2013

    The changeset 2e446e87ac5b broke some buildbots at compile step.

    ./python -E -S -m sysconfig --generate-posix-vars
    Could not find platform dependent libraries <exec_prefix>
    Consider setting $PYTHONHOME to <prefix>[:<exec_prefix>]
    Fatal Python error: Py_Initialize: can't initialize sys standard streams
    Traceback (most recent call last):
      File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/locale.py", line 17, in <module>
        import re
      File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/re.py", line 124, in <module>
        import functools
      File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/functools.py", line 18, in <module>
        from collections import namedtuple
      File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/collections/__init__.py", line 8, in <module>
        __all__ += collections.abc.__all__
    AttributeError: 'module' object has no attribute 'abc'

    http://buildbot.python.org/all/builders/x86%20RHEL%206%203.x/builds/1963

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 5, 2013

    New changeset 186cf551dae5 by Richard Oudkerk in branch 'default':
    Issue bpo-15528: Add weakref.finalize to support finalization using
    http://hg.python.org/cpython/rev/186cf551dae5

    @sbt sbt mannequin closed this as completed May 5, 2013
    @ambv
    Copy link
    Contributor

    ambv commented Jun 8, 2013

    Your patch leaks references with subinterpreters, which was exposed by functools.singledispatch using weakref. While the actual bug is in atexit (which doesn't properly unregister on subinterpreter termination), now all uses of weakref leak.

    Context: http://mail.python.org/pipermail/python-dev/2013-June/126755.html

    PJE suggests importing atexit and registering finalize only when it's actually used. I guess this would be the easiest workaround.

    @ambv ambv reopened this Jun 8, 2013
    @ambv ambv assigned sbt Jun 8, 2013
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 8, 2013

    New changeset d6ad9d7468f7 by Richard Oudkerk in branch 'default':
    Issue bpo-15528: Delay importing atexit until weakref.finalize() used.
    http://hg.python.org/cpython/rev/d6ad9d7468f7

    @sbt
    Copy link
    Mannequin Author

    sbt mannequin commented Jun 8, 2013

    PJE suggests importing atexit and registering finalize only when it's
    actually used. I guess this would be the easiest workaround.

    Done.

    @sbt sbt mannequin closed this as completed Jun 8, 2013
    @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
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants