Better support for finalization with weakrefs

[AntoinePitrou]

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst
File Doc/library/weakref.rst (right):

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst#newcode255
Doc/library/weakref.rst:255: These are called in reverse order of creation.
You need a versionadded tag on the documentatioN.

http://bugs.python.org/review/15528/diff/5627/Lib/test/test_weakref.py
File Lib/test/test_weakref.py (right):

http://bugs.python.org/review/15528/diff/5627/Lib/test/test_weakref.py#newcod...
Lib/test/test_weakref.py:1467: output = subprocess.check_output([sys.executable,
'-c', prog])
You should use the helpers from test.script_helper instead.

http://bugs.python.org/review/15528/diff/5627/Lib/test/test_weakref.py#newcod...
Lib/test/test_weakref.py:1555: >>> import weakref
My personal opinion about doctests is that we shouldn't add any more of them,
since they're tedious to maintain. Also, it doesn't seem they're testing
anything that's not already tested by the unit tests above.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py
File Lib/weakref.py (right):

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode427
Lib/weakref.py:427: def pop(self):
I agree that detach() sounds better.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode434
Lib/weakref.py:434: def get(self):
Is there any point to this method?

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode450
Lib/weakref.py:450: v.__traceback__ = tb
I think it's better to use the with_traceback() method.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode451
Lib/weakref.py:451: sys.excepthook(t, v, tb)
Why do you invoke the excepthook yourself instead of re-raising the exception?

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode455
Lib/weakref.py:455: # Used by _atexit_callback() - overridable by subclasses
Overridability sounds overkill to me.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode459
Lib/weakref.py:459: # Overridable by subclasses
Same here.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode469
Lib/weakref.py:469: f._make_wrapper()(None)
If invoking a finalizer creates another finalizer, it won't be executed here.
You probably want another loop around this.

[sbt]

Thanks for the review Antoine.

http://bugs.python.org/review/15528/diff/5627/Lib/test/test_weakref.py
File Lib/test/test_weakref.py (right):

http://bugs.python.org/review/15528/diff/5627/Lib/test/test_weakref.py#newcod...
Lib/test/test_weakref.py:1555: >>> import weakref
On 2012/08/02 23:19:01, AntoinePitrou wrote:
> My personal opinion about doctests is that we shouldn't add any more of them,
> since they're tedious to maintain. Also, it doesn't seem they're testing
> anything that's not already tested by the unit tests above.
I did wonder whether it would be better to make the unittests pass
Doc/library/weakref.rst to doctest.  (It passes.)

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py
File Lib/weakref.py (right):

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode427
Lib/weakref.py:427: def pop(self):
On 2012/08/02 23:19:01, AntoinePitrou wrote:
> I agree that detach() sounds better.
Calvin also suggested returning obj instead of wr in the tuple, which I think
would be an improvement.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode434
Lib/weakref.py:434: def get(self):
On 2012/08/02 23:19:01, AntoinePitrou wrote:
> Is there any point to this method?
get() lets you non-destructively test whether the finalizer is alive.  But an
isalive() method might be just as good.  The Fd class at the end of the examples
uses it for that.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode451
Lib/weakref.py:451: sys.excepthook(t, v, tb)
On 2012/08/02 23:19:01, AntoinePitrou wrote:
> Why do you invoke the excepthook yourself instead of re-raising the exception?
Because raising from a weakref callback does not produce a traceback.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode455
Lib/weakref.py:455: # Used by _atexit_callback() - overridable by subclasses
On 2012/08/02 23:19:01, AntoinePitrou wrote:
> Overridability sounds overkill to me.
I hope to replace multiprocessing.util.Finalize with a subclass of
weakref.finalize.  For that I would also need support for atexit priorities, and
pid checks.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode469
Lib/weakref.py:469: f._make_wrapper()(None)
On 2012/08/02 23:19:01, AntoinePitrou wrote:
> If invoking a finalizer creates another finalizer, it won't be executed here.
> You probably want another loop around this.
Presumably with a limit to the number of retries?

[AntoinePitrou]

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py
File Lib/weakref.py (right):

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode451
Lib/weakref.py:451: sys.excepthook(t, v, tb)
On 2012/08/03 01:02:14, sbt wrote:
> On 2012/08/02 23:19:01, AntoinePitrou wrote:
> > Why do you invoke the excepthook yourself instead of re-raising the
exception?
> Because raising from a weakref callback does not produce a traceback.

True, but I think that's a bug we want to fix. See
http://bugs.python.org/issue7317

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode455
Lib/weakref.py:455: # Used by _atexit_callback() - overridable by subclasses
On 2012/08/03 01:02:14, sbt wrote:
> On 2012/08/02 23:19:01, AntoinePitrou wrote:
> > Overridability sounds overkill to me.
> I hope to replace multiprocessing.util.Finalize with a subclass of
> weakref.finalize.  For that I would also need support for atexit priorities,
and
> pid checks.

Ah, fair enough.

http://bugs.python.org/review/15528/diff/5627/Lib/weakref.py#newcode469
Lib/weakref.py:469: f._make_wrapper()(None)
On 2012/08/03 01:02:14, sbt wrote:
> On 2012/08/02 23:19:01, AntoinePitrou wrote:
> > If invoking a finalizer creates another finalizer, it won't be executed
here.
> > You probably want another loop around this.
> Presumably with a limit to the number of retries?

If you like, but I'm not sure that's useful.

[storchaka]

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst
File Doc/library/weakref.rst (right):

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst#newcode483
Doc/library/weakref.rst:483: weakref.finalize(self, os.close, fd)
def __del__(self):
    os.close(self)

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst#newcode492
Doc/library/weakref.rst:492: class Fd(int):
class Fd(int):

    closed = False

    def __del__(self):
        self.close()

    def close(self):
        if not self.closed:
            os.close(self)
            self.closed = True

    def detach(self):
        if self.closed:
            raise ValueError("already closed or detached")
        self.closed = True
        return int(self)

[sbt]

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst
File Doc/library/weakref.rst (right):

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst#newcode483
Doc/library/weakref.rst:483: weakref.finalize(self, os.close, fd)
On 2012/08/04 21:50:01, storchaka wrote:
> def __del__(self):
>     os.close(self)
I did not say you could not manage this with __del__.

But your code will likely throw an exception if it gets called during shutdown
because os == None.  This is one problem that using finalize() avoids.

Your alternative *could* be fixed by doing

def __del__(self, close=os.close):
    close(self)

http://bugs.python.org/review/15528/diff/5627/Doc/library/weakref.rst#newcode492
Doc/library/weakref.rst:492: class Fd(int):
On 2012/08/04 21:50:01, storchaka wrote:
> class Fd(int):
>     ...
Not a major issue, but this is not thread safe.  The finalize alternative is
(because dict.pop() is atomic for "simple" keys).

[asvetlov]

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py
File Lib/weakref.py (right):

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py#newcode415
Lib/weakref.py:415: __slots__ = ("wr", "func", "args", "kwds", "atexit",
"count")
`kwds` usually named as `kwargs`, the rather that you already have `args`.
`count` is really `counter` maybe? For me `count` means "number of items for
something".
Maybe rename `wr` to `weakref` or `ref` to make code more self explained?

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py#newcode494
Lib/weakref.py:494: if not pending:
using pending which initialized only if _dirty is True doesn't look clean and
obvious.

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py#newcode508
Lib/weakref.py:508: gc and gc.enable()
Perhaps gc should not to be enabled if it was disabled before _exitfunc been
called.

[sbt]

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py
File Lib/weakref.py (right):

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py#newcode415
Lib/weakref.py:415: __slots__ = ("wr", "func", "args", "kwds", "atexit",
"count")
On 2012/08/19 16:58:32, asvetlov wrote:
> `kwds` usually named as `kwargs`, the rather that you already have `args`.
> `count` is really `counter` maybe? For me `count` means "number of items for
> something".
> Maybe rename `wr` to `weakref` or `ref` to make code more self explained?

Maybe I will rethink the names.  But since this is not part of the API, I don't
think it really matters.

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py#newcode494
Lib/weakref.py:494: if not pending:
On 2012/08/19 16:58:32, asvetlov wrote:
> using pending which initialized only if _dirty is True doesn't look clean and
> obvious.
Okay.

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py#newcode508
Lib/weakref.py:508: gc and gc.enable()
On 2012/08/19 16:58:32, asvetlov wrote:
> Perhaps gc should not to be enabled if it was disabled before _exitfunc been
> called.
I guess so.

[asvetlov]

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py
File Lib/weakref.py (right):

http://bugs.python.org/review/15528/diff/5761/Lib/weakref.py#newcode415
Lib/weakref.py:415: __slots__ = ("wr", "func", "args", "kwds", "atexit",
"count")
On 2012/08/19 19:25:58, sbt wrote:
> Maybe I will rethink the names.  But since this is not part of the API, I
don't
> think it really matters.
I thought finalize.Info *is* the part of API as it name doesn't start from
underscore. Well, you never return Info objects and these stored into "private"
`_registry` classattr.

Please forgive me my nagging but I think the clean public API names are matters.

Also implicit `return None` in __call__, detach and peek looks a bit tricky for
newbie reader. Anyway docstrings should directly specify when None returned.

[asvetlov]

http://bugs.python.org/review/15528/diff/5775/Doc/library/weakref.rst
File Doc/library/weakref.rst (right):

http://bugs.python.org/review/15528/diff/5775/Doc/library/weakref.rst#newcode487
Doc/library/weakref.rst:487: __index__ = __int__ = fileno
I think this line isn't required for docs

http://bugs.python.org/review/15528/diff/5775/Lib/weakref.py
File Lib/weakref.py (right):

http://bugs.python.org/review/15528/diff/5775/Lib/weakref.py#newcode487
Lib/weakref.py:487: reenable_gc = False
reenable_gc = None

http://bugs.python.org/review/15528/diff/5775/Lib/weakref.py#newcode492
Lib/weakref.py:492: reenable_gc = True
reenable_gc = gc.enable

http://bugs.python.org/review/15528/diff/5775/Lib/weakref.py#newcode513
Lib/weakref.py:513: if reenable_gc:
if reenable_gc is not None:
    reenable_gc()