Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loggers can't be pickled #74705

Closed
pitrou opened this issue May 31, 2017 · 7 comments
Closed

loggers can't be pickled #74705

pitrou opened this issue May 31, 2017 · 7 comments
Assignees
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented May 31, 2017

BPO 30520
Nosy @vsajip, @pitrou, @serhiy-storchaka
PRs
  • bpo-30520: Implemented pickling for loggers. #1956
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/vsajip'
    closed_at = <Date 2017-06-06.15:39:08.500>
    created_at = <Date 2017-05-31.07:50:48.665>
    labels = ['3.7', 'type-feature', 'library']
    title = "loggers can't be pickled"
    updated_at = <Date 2017-06-06.15:39:08.499>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2017-06-06.15:39:08.499>
    actor = 'vinay.sajip'
    assignee = 'vinay.sajip'
    closed = True
    closed_date = <Date 2017-06-06.15:39:08.500>
    closer = 'vinay.sajip'
    components = ['Library (Lib)']
    creation = <Date 2017-05-31.07:50:48.665>
    creator = 'pitrou'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30520
    keywords = []
    message_count = 7.0
    messages = ['294818', '294907', '294908', '294921', '294923', '294930', '295265']
    nosy_count = 3.0
    nosy_names = ['vinay.sajip', 'pitrou', 'serhiy.storchaka']
    pr_nums = ['1956']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue30520'
    versions = ['Python 3.7']

    @pitrou
    Copy link
    Member Author

    pitrou commented May 31, 2017

    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 "<ipython-input-4-6ecead831873>", line 1, in <module>
        pickle.dumps(log)
    TypeError: can't pickle _thread.RLock objects

    @pitrou pitrou added 3.7 (EOL) end of life stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 31, 2017
    @vsajip
    Copy link
    Member

    vsajip commented Jun 1, 2017

    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.

    @pitrou
    Copy link
    Member Author

    pitrou commented Jun 1, 2017

    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).

    @serhiy-storchaka
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member Author

    pitrou commented Jun 1, 2017

    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.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @vsajip
    Copy link
    Member

    vsajip commented Jun 6, 2017

    New changeset 6260d9f by Vinay Sajip in branch 'master':
    bpo-30520: Implemented pickling for loggers. (bpo-1956)
    6260d9f

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants