classification
Title: multiprocessing logging support test
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: jnoller Nosy List: OG7, amaury.forgeotdarc, flox, jnoller, pitrou, vinay.sajip
Priority: normal Keywords: patch

Created on 2009-08-01 16:09 by OG7, last changed 2010-02-09 14:47 by vinay.sajip. This issue is now closed.

Files
File name Uploaded Description Edit
6615.patch asksol, 2009-11-21 13:51 applies cleanly to trunk.
issue6615_weakref.diff flox, 2009-11-25 00:14 Patch using weakref (trunk rev 76505)
Messages (23)
msg91163 - (view) Author: OG7 (OG7) Date: 2009-08-01 16:09
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).
msg95584 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2009-11-21 14:38
patch committed to trunk in r76438
msg95588 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2009-11-21 18:10
merged to py3k in r76439
msg95677 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2009-11-24 14:22
I've commented out the test (therefore, reopening this) the test 
introduces a pretty bad refleak problem. Need to debug.
msg95679 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-11-24 14:27
Are these leaks caused by the test, or revealed by it?
msg95680 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-24 15:05
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]
msg95681 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-11-24 15:14
Yes, test_logging has a serious tearDown() method to wipe installed
handlers.
msg95682 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-24 15:16
> 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.
msg95685 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2009-11-24 16:40
Yeah, I should have checked the tearDown stuff in the logging test suite
msg95691 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2009-11-24 18:01
> 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]
>>>
msg95694 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-24 18:08
> 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.
msg95695 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2009-11-24 18:23
> 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.
msg95696 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2009-11-24 18:24
> removing from a handler), it's removed from the list and would not cause a 

s/handler/logger/
msg95697 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2009-11-24 19:23
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).
msg95698 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2009-11-24 20:47
> 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...
msg95701 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2009-11-24 22:41
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()
msg95704 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-24 22:46
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
msg95705 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2009-11-24 23:55
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.
msg95706 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2009-11-25 00:14
Small change to acquire the module lock before working on _handlerList.
msg95713 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2009-11-25 09:38
Changes checked into trunk (r76508) - very slightly different to flox's patch. Also made _handlers a WeakValueDictionary.
msg95715 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2009-11-25 10:45
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()
msg95718 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2009-11-25 14:13
> 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).
msg99118 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-02-09 14:33
It should be closed now.
History
Date User Action Args
2010-02-09 14:47:20vinay.sajipsetstatus: pending -> closed
resolution: accepted -> fixed
2010-02-09 14:34:28floxsetstatus: open -> pending
2010-02-09 14:33:47floxsetstatus: pending -> open
versions: - Python 2.6, Python 3.1
2010-02-09 14:33:16floxsetstatus: open -> pending
priority: normal
messages: + msg99118

stage: resolved
2009-11-25 14:13:12vinay.sajipsetmessages: + msg95718
2009-11-25 10:45:23floxsetmessages: + msg95715
versions: + Python 3.1, Python 2.7, Python 3.2
2009-11-25 09:38:50vinay.sajipsetmessages: + msg95713
2009-11-25 00:14:38floxsetfiles: - issue6615_weakref.diff
2009-11-25 00:14:16floxsetfiles: + issue6615_weakref.diff

messages: + msg95706
2009-11-24 23:57:32floxsetfiles: - issue6615_weakref.diff
2009-11-24 23:55:50floxsetfiles: + issue6615_weakref.diff

messages: + msg95705
2009-11-24 22:46:52pitrousetmessages: + msg95704
2009-11-24 22:41:46floxsetfiles: + issue6615_weakref.diff

messages: + msg95701
2009-11-24 21:54:16floxsetfiles: - issue6615_weakref.diff
2009-11-24 20:47:47vinay.sajipsetmessages: + msg95698
2009-11-24 19:23:48floxsetfiles: + issue6615_weakref.diff
nosy: + flox
messages: + msg95697

2009-11-24 18:24:57vinay.sajipsetmessages: + msg95696
2009-11-24 18:23:46vinay.sajipsetmessages: + msg95695
2009-11-24 18:08:35pitrousetmessages: + msg95694
2009-11-24 18:01:41vinay.sajipsetmessages: + msg95691
2009-11-24 16:40:13jnollersetmessages: + msg95685
2009-11-24 15:16:54pitrousetmessages: + msg95682
2009-11-24 15:14:25amaury.forgeotdarcsetmessages: + msg95681
2009-11-24 15:05:25pitrousetnosy: + pitrou, vinay.sajip
messages: + msg95680
2009-11-24 14:27:34amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg95679
2009-11-24 14:22:14jnollersetstatus: closed -> open
resolution: fixed -> accepted
messages: + msg95677
2009-11-21 18:10:25jnollersetstatus: open -> closed
resolution: fixed
messages: + msg95588
2009-11-21 14:38:57jnollersetmessages: + msg95584
2009-11-21 13:51:18asksolsetfiles: + 6615.patch
keywords: + patch
2009-08-01 21:46:10jnollersetassignee: jnoller
2009-08-01 16:09:03OG7create