| 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) *  |
Date: 2009-11-21 14:38 |
| patch committed to trunk in r76438 |
|
|
| msg95588 - (view) |
Author: Jesse Noller (jnoller) * |
Date: 2009-11-21 18:10 |
| merged to py3k in r76439 |
|
|
| msg95677 - (view) |
Author: Jesse Noller (jnoller) * |
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) * |
Date: 2009-11-24 14:27 |
| Are these leaks caused by the test, or revealed by it? |
|
|
| msg95680 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
Date: 2009-11-24 15:14 |
| Yes, test_logging has a serious tearDown() method to wipe installed handlers. |
|
|
| msg95682 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2009-11-25 00:14 |
| Small change to acquire the module lock before working on _handlerList. |
|
|
| msg95713 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
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) * |
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) * |
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) * |
Date: 2010-02-09 14:33 |
| It should be closed now. |
|
|