| msg294818 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2017-05-31 07:50 |
| Loggers could simply be pickled and unpickled by name, but pickle currently tries the hard way: >>> import pickle >>> import logging >>> log = logging.getLogger('foo') >>> pickle.dumps(log) Traceback (most recent call last): File "", line 1, in pickle.dumps(log) TypeError: can't pickle _thread.RLock objects |
|
|
| msg294907 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2017-06-01 07:14 |
| I am not sure it is a good idea to support pickling of loggers, as they are singletons and generally aren't supposed to be instantiated other than by calling logging.getLogger(name). What's the use case for pickling them, giving that (as you say) just the name could be used? In general, it's not needed to have loggers as part of an object instance, so I don't quite see where the need to pickle comes from. One could implement __getstate__() to just return the name, but there is no corresponding obvious implementation of __setstate__() because loggers aren't meant to be instantiated via unpickling. |
|
|
| msg294908 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-06-01 07:18 |
| Le 01/06/2017 à 09:14, Vinay Sajip a écrit : > > I am not sure it is a good idea to support pickling of loggers, as they are singletons and generally aren't supposed to be instantiated other than by calling logging.getLogger(name). Pickling them by name is precisely what I'm having in mind. Right now, pickle tries to recreate them structurally, which fails. In other words (untested, but you get the idea): class Logger: def __reduce__(self): return getLogger, (self.name,) > What's the use case for pickling them, You usually don't pickle loggers directly. You pickle an object that happens to hold (directly or indirectly) a reference to a logger (it's quite common to have `self.logger = ...` in your code), and pickle tries to pickle the logger as part of pickling that object. > One could implement __getstate__() to just return the name, but there is no corresponding obvious implementation of __setstate__() because loggers aren't meant to be instantiated via unpickling. No need for __getstate__ or __setstate__, __reduce__ should work fine (see above). |
|
|
| msg294921 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-06-01 09:28 |
| The idea LGTM. But we should first check that the logger is accessible by the name: getLogger(self.name) is self. The same is done when pickling classes, functions, etc. It shouldn't be a surprise on unpickler side. And maybe use not getLogger(), but different method which should fail if the logger doesn't exist rather than creating it. Otherwise this looks as pickling open files by name. |
|
|
| msg294923 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-06-01 09:38 |
| Le 01/06/2017 à 11:28, Serhiy Storchaka a écrit : > > The idea LGTM. But we should first check that the logger is accessible by the name: getLogger(self.name) is self. The same is done when pickling classes, functions, etc. It shouldn't be a surprise on unpickler side. Good idea. > And maybe use not getLogger(), but different method which should fail if the logger doesn't exist rather than creating it. I disagree. The fact that it creates a new logger if it doesn't exist is pretty much by design. Typically, if you have: class MyObject: def __init__(self): self.logger = getLogger('myobject') then unpickling the first MyObject instance in a new process should also create the 'myobject' logger instead of failing. |
|
|
| msg294930 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-06-01 10:27 |
| > The fact that it creates a new logger if it doesn't exist is > pretty much by design. May be. I don't have a strong opinion. |
|
|
| msg295265 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2017-06-06 15:34 |
| New changeset 6260d9f2039976372e0896d517b3c06e606eb169 by Vinay Sajip in branch 'master': bpo-30520: Implemented pickling for loggers. (#1956) https://github.com/python/cpython/commit/6260d9f2039976372e0896d517b3c06e606eb169 |
|
|