homepage

There is an additional test for multiprocessing's logging support here:
http://code.google.com/p/python-multiprocessing/issues/detail?id=18
(disregard the fix, it is only needed for the backport).

patch committed to trunk in r76438 

merged to py3k in r76439 

I've commented out the test (therefore, reopening this) the test 
introduces a pretty bad refleak problem. Need to debug.

Are these leaks caused by the test, or revealed by it?

From a quick command-line attempt, it seems that logging is the culprit:
>>> handler = logging.Handler()
[64370 refs]
>>> handler = logging.Handler()
[64383 refs]
>>> handler = logging.Handler()
[64396 refs]

Yes, test_logging has a serious tearDown() method to wipe installed
handlers.

> Yes, test_logging has a serious tearDown() method to wipe installed
> handlers.
In general it would be nice if logging were more conservative (or
cautious) when it comes to keeping objects persistent. It is too easy to
fall into a trap and experience memory leaks.

Yeah, I should have checked the tearDown stuff in the logging test suite

> In general it would be nice if logging were more conservative (or
> cautious) when it comes to keeping objects persistent. It is too easy to
> fall into a trap and experience memory leaks.
I've checked in a change into trunk today which might improve matters. Handler.__init__ no longer adds the handler instance to the _handlers map unconditionally, but still adds it to the _handlerList array. The only place this array is actually used is in the shutdown() API, which is registered to be run via atexit. It flushes and closes all the handlers in the list, so that any data in logging buffers gets flushed before the process exits.
The _handlers dict is now used to hold a mapping between handler names and handlers - the name property was added to Handler in the change I checked in today. This will be used by the dictConfig functionality which is proposed in PEP 391. If no name property is set on a Handler, then this will not add any additional references to handlers.
Python 2.7a0 (trunk:75403, Oct 14 2009, 20:14:09) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import logging
[49139 refs]
>>> logging._handlerList
[]
[49146 refs]
>>> logging.Handler()
<logging.Handler object at 0x00B72658>
[49179 refs]
>>> logging.Handler()
<logging.Handler object at 0x00B72620>
[49202 refs]
>>> logging.Handler()
<logging.Handler object at 0x00B764D0>
[49225 refs]
>>> logging._handlerList
[<logging.Handler object at 0x00B764D0>, <logging.Handler object at 0x00B72620>,
 <logging.Handler object at 0x00B72658>]
[49225 refs]
>>> logging.shutdown()
[49156 refs]
>>> logging._handlerList
[]
[49156 refs]
>>>

> Handler.__init__ no longer adds the handler instance to the _handlers
> map unconditionally, but still adds it to the _handlerList array. The
> only place this array is actually used is in the shutdown() API, which
> is registered to be run via atexit. It flushes and closes all the
> handlers in the list, so that any data in logging buffers gets flushed
> before the process exits.
Why is this exactly? Why do you want to keep handlers until shutdown
rather than dispose of them when they aren't used anymore?
Moreover, closing in atexit is rather bad because the resources
necessary for proper closing (files, sockets...) may already have been
freed.

> Why is this exactly? Why do you want to keep handlers until shutdown
> rather than dispose of them when they aren't used anymore?
Typically handlers are only instantiated when being added to loggers, and loggers live for the lifetime of the process so those handler references will typically be persistent. However, if a handler is closed (typically after removing from a handler), it's removed from the list and would not cause a memory leak.
> Moreover, closing in atexit is rather bad because the resources
> necessary for proper closing (files, sockets...) may already have been
> freed.
That's potentially true, but it's behaviour which has been there from the beginning so ISTM it needs to be there for backward compatibility reasons :-( When logging.raiseExceptions is True (the default) any exceptions in shutdown() would be raised, but this doesn't appear to happen in practice.

> removing from a handler), it's removed from the list and would not cause a 
s/handler/logger/

I would think to use weak references in order to prevent such leaks.
With the patch attached, if the handler is not referenced anywhere, it
is closed.
However named handlers are not closed (because of hard reference in
_handlers dictionary).

> With the patch attached, if the handler is not referenced anywhere, it
> is closed.
> However named handlers are not closed (because of hard reference in
> _handlers dictionary).
I haven't tried it yet, but does the patch actually work? You seem to have self_weakref in one place and self._weakref in another...

yep... patch was not clean. Sorry :(
I changed it.
It passes the 21 tests of the test_logging suite.
And the count of references decreases with the test:
>>> import logging
>>> handler = logging.Handler()
>>> handler = logging.Handler()

Some quick comments on your patch (not an in-depth review):
- you should add some tests for the problem you're trying to solve
- using __del__ when you have a weakref is counter-productive; use the
weakref's optional callback instead
- if you remove arbitrary elements from it, _handlerList should probably
be a set rather a list (but it's more of an optimization concern)
- `for h in [wr() for wr in handlerList if wr() is not None]` isn't a
pretty notation; just put the `if` inside the `for` instead

Updated the patch with technical recommendations from Antoine.
I kept the _handlerList as a list because "It allows handlers to be
removed in reverse of order initialized."
And some tests are needed to outline the change.

Small change to acquire the module lock before working on _handlerList.

Changes checked into trunk (r76508) - very slightly different to flox's patch. Also made _handlers a WeakValueDictionary.

Thank you Vinay.
Since you "reversed()" the _handlerList, the following part need to be
changed:
Index: Lib/logging/__init__.py
===================================================================
--- Lib/logging/__init__.py (revision 76508)
+++ Lib/logging/__init__.py (working copy)
@@ -610,7 +610,7 @@
 """
 _acquireLock()
 try:
- _handlerList.insert(0, weakref.ref(handler, _removeHandlerRef))
+ _handlerList.append(weakref.ref(handler, _removeHandlerRef))
 finally:
 _releaseLock()

> Since you "reversed()" the _handlerList, the following part need to be
> changed:
> 
> - _handlerList.insert(0, weakref.ref(handler, _removeHandlerRef))
> + _handlerList.append(weakref.ref(handler, _removeHandlerRef))
Corrected in r76509. Florent, thanks for catching this (and for the patch).

It should be closed now.