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

Sentinels identity lost when pickled (unittest.mock) #65003

Closed
VlastimilZma mannequin opened this issue Feb 28, 2014 · 28 comments
Closed

Sentinels identity lost when pickled (unittest.mock) #65003

VlastimilZma mannequin opened this issue Feb 28, 2014 · 28 comments
Labels
3.7 (EOL) end of life tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@VlastimilZma
Copy link
Mannequin

VlastimilZma mannequin commented Feb 28, 2014

BPO 20804
Nosy @bitdancer, @voidspace, @serhiy-storchaka, @applio
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • pickle_sentinels.py
  • mock_sentinel_pickle.patch
  • sentinel-doc.patch
  • mock_sentinel_pickle2.patch
  • 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 = None
    closed_at = <Date 2017-01-11.18:23:07.757>
    created_at = <Date 2014-02-28.09:22:37.196>
    labels = ['3.7', 'type-feature', 'tests']
    title = 'Sentinels identity lost when pickled (unittest.mock)'
    updated_at = <Date 2017-03-31.16:36:24.480>
    user = 'https://bugs.python.org/VlastimilZma'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:24.480>
    actor = 'dstufft'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-01-11.18:23:07.757>
    closer = 'serhiy.storchaka'
    components = ['Tests']
    creation = <Date 2014-02-28.09:22:37.196>
    creator = 'Vlastimil.Z\xc3\xadma'
    dependencies = []
    files = ['34256', '46245', '46253', '46260']
    hgrepos = []
    issue_num = 20804
    keywords = ['patch']
    message_count = 28.0
    messages = ['212417', '212434', '212440', '212451', '212452', '212454', '212455', '212460', '212461', '212462', '212476', '212481', '212621', '212633', '285152', '285156', '285161', '285202', '285203', '285208', '285212', '285239', '285241', '285244', '285249', '285251', '285252', '285254']
    nosy_count = 7.0
    nosy_names = ['peter.otten', 'r.david.murray', 'michael.foord', 'python-dev', 'serhiy.storchaka', 'Vlastimil.Z\xc3\xadma', 'davin']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20804'
    versions = ['Python 3.7']

    @VlastimilZma
    Copy link
    Mannequin Author

    VlastimilZma mannequin commented Feb 28, 2014

    Sentinels lose their identity when they are pickled, meaning they are no longer equal to sentinel with the same name.

    >>> from mock import sentinel
    >>> import pickle
    >>> sentinel.foo == sentinel.foo
    True
    >>> pickle.loads(pickle.dumps(sentinel.foo)) == sentinel.foo
    False

    I found this bug using Python 2.7.3 and python-mock 1.0.1. Since mock 1.0 is part of Python 3.3 and 3.4 it affects these versions (tested in 3.3.3).

    This behavior is tricky to find out if encountered in tests and sentinels can not be used in tests of code which uses pickle and possibly other serializations.

    @VlastimilZma VlastimilZma mannequin added type-bug An unexpected behavior, bug, or error tests Tests in the Lib/test dir labels Feb 28, 2014
    @bitdancer
    Copy link
    Member

    Since the point of a sentinel is *object identity*, there is no way that you can get the "same" sentinel back after a pickle. This is fundamental to the nature of pickle.

    @VlastimilZma
    Copy link
    Mannequin Author

    VlastimilZma mannequin commented Feb 28, 2014

    I don't question pickle, but I'd expect sentinels to keep their equality when they are pickled or copied.

    @bitdancer
    Copy link
    Member

    Sentinels are not equal unless they are the *same object*, just as for any python object that does not have a defined __eq__. And as I said, this is part of the point of sentinels.

    @voidspace
    Copy link
    Contributor

    Maybe making sentinels unpickleable would be a better solution. On the other hand I'd love to be able to [properly] customise object creation when unpickling. It would solve various other problems I've had from time to time. But that's well outside the scope of this bug.

    @bitdancer
    Copy link
    Member

    In case anyone is wondering, I'm leaving this open in case Michael thinks it is a good idea to add some sort of documentation note about this. I'm not sure it is, because this is fundamental to the way Python works. On the other hand, I can't think of any specific manual or tutorial section we could direct someone to to get an explanation of what is going on.

    This one comes close: http://docs.python.org/2/faq/programming.html#how-can-my-code-discover-the-name-of-an-object but doesn't tell the whole story since it doesn't cover pickle.

    @bitdancer
    Copy link
    Member

    Our posts crossed.

    Making them unpickleable sounds like it might be sensible. Since the sentinel *can't* survive pickling/unpickling and still perform its documented function, it seems better to raise an error if someone tries.

    @peterotten
    Copy link
    Mannequin

    peterotten mannequin commented Feb 28, 2014

    From looking at the sentinel code it appears a sentinel's identity is controlled by its name alone, i. e.

    sentinel.foo is sentinel.foo

    If that's the desired behaviour it is well possible to make that indentity survive pickling. I have attached a demo script using the simplest approach -- registering the class with copyreg.
    If you like the general idea I'm willing to look up the alternative (a __getstate__() or __reduce__() method), too ;)

    @bitdancer
    Copy link
    Member

    Personally I think that's a bad idea. Anything that implies that object identity can survive pickle/unpickle is IMO bad, since pickle is a serialization protocol and in real life (as opposed to tests) the process that does the unpickle is almost always not the process that does the pickle.

    @peterotten
    Copy link
    Mannequin

    peterotten mannequin commented Feb 28, 2014

    I have no experience with unittest.mock, so I'm in no position to contradict... Vlastimil, could you give your use case?

    @VlastimilZma
    Copy link
    Mannequin Author

    VlastimilZma mannequin commented Feb 28, 2014

    I encountered this problem when I tested code with cache, something like

        def test_foo(self):
            cache.set('a_key', sentinel.status) # Performs pickle
            self.assertEqual(
                cache.get('a_key'), # Performs unpickle
                sentinel.status)

    I spent some time searching for reason why the test failed. Since both sentinels represent themselves as 'sentinel.status', I was quite surprised by assertion failure. Only after I looked at the code I realized where is the problem. Two sentinels are equal if and only if they are identical.

    On my way home I realized this probably is and needs to be a feature of sentinels. Consider testing this function:

        def foo(value, do_copy):
            if do_copy:
                return bar(copy(value))
            else:
                return bar(value)

    You would like to know whether bar was called with value or its copy. Sentinel is in a test for such function a great option to make sure value is not copied in process. This is especially usefull is you write tests for wrapper-like function which should only pass arguments (or a subset) to other interface.

    After reading through comments and thinking about the problem I have following opinions:

    • Sentinels should not actually survive pickle/unpickle.
    • Prevent sentinels from being pickled is a good idea, it is a valid way to tell developer he does something he shouldn't. This might also apply to copying, which would result in the same problem.
    • It should be noted in documentation that sentinels are equal only if identical. Since they represent themselves by their name it is hard to guess the name is only for the representation. Also I find the documentation snippet

    >> assert result is sentinel.some_object

    somewhat misleading as you don't usually use 'is' for comparison and it provides no information about equality.

    @bitdancer
    Copy link
    Member

    Yes, exactly, you don't use is for equality comparison. It tests object identity, which is why it makes sense to use it with a sentinel.

    The 'name' of the sentinel is purely a way to store and retrieve particular sentinel objects. The sentinel itself does not have a name.

    Good point about copy...the copy protocol and the pickle protocol are closely related, so preventing a sentinel from being pickled may prevent it from being copied as well. It is an interesting question how surprising people will find it for a sentinel to throw an error if an attempt is made to copy it, but if the test is using a sentinel, presumably it expects the sentinel to survive unchanged, so an error on copy is probably a reasonable result. You could imagine someone wanting to use a sentinel to test that copy happens by making sure they get back a different object, though, so it is not entirely clear cut. However, I expect that to be *much* less common than wanting to use it to prove that an object is preserved (not copied).

    @VlastimilZma
    Copy link
    Mannequin Author

    VlastimilZma mannequin commented Mar 3, 2014

    David, first part of your comment should be added to docs. Purpose of sentinels isn't much intuitive and in my opinion documentation doesn't quite describe it.

    An exception in case of pickle or copy would be much easier to understand apart from unexpected inequality. In my experience negative test in general are uncommon, so I expect usage of sentinels to ensure different objects to be rare.

    @voidspace
    Copy link
    Contributor

    I'm not sure to what extent mock documentation should explain Python semantics. The docs *do* make clear that sentinel is useful for where you want to test (compare) objects by *identity*. Problems with copying and pickling them come from the fact that those operations don't preserve *identity*, which is a basic facet of *Python* and not down to mock/sentinel.

    "sentinel provides a convenient way of creating and testing the identity of objects like this."

    @serhiy-storchaka
    Copy link
    Member

    Proposed patch makes mock sentinels supporting pickling and copying. This allows passing them in multiprocessing.

    I'm not sure whether this is a bug fix or a new feature.

    The patch works only with 3.5+. Older versions need more complex solution.

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Jan 10, 2017
    @applio
    Copy link
    Member

    applio commented Jan 10, 2017

    Serhiy: The above discussion seemed to converge on the perspective that object identity should not survive pickling and that the point of a sentinel is object identity. While your proposed patch may mechanically work, I believe it is in conflict with the outcome of the thoughtful discussion above.

    @serhiy-storchaka
    Copy link
    Member

    Sorry, I had wrote this patch for bpo-29229 and didn't read the above discussion.

    Copying and pickling preserve identity of some objects: functions, classes, enums, builtin singletons (None, False, True, ...). It was not obvious to me that this shouldn't work with mock sentinels.

    @VlastimilZma
    Copy link
    Mannequin Author

    VlastimilZma mannequin commented Jan 11, 2017

    Since the issue was dug up, I've created a patch for documentation.

    I still think, that it is worth stating the equality is not preserved for sentinels when copied as the message produced by tests is rather confusing:

        class SentinelTestCase(TestCase):
            def test_sentinel(self):
                self.assertEqual(sentinel.foo, copy(sentinel.foo))
                # AssertionError: sentinel.foo != sentinel.foo

    @voidspace
    Copy link
    Contributor

    So my thinking has evolved slightly on it. If it's *possible* for sentinels to be copied and pickled and preserve identity then I'm happy for it. I think the right semantics for copying a sentinel is that you get the original object back. If you pickle a sentinel and then unpickle back *into the same process* you should get the same object back.

    David worried that this confuses the semantics of pickling/copying because it isn't generally the case - so I'm open to further discussion on it, but if Serhiy has fixed the "problem" here I'm happy for it to go in.

    @serhiy-storchaka
    Copy link
    Member

    I just don't know well the purpose of sentinels, and don't . I see fourth possible options:

    1. Raise an exception when copy or pickle a sentinel. This is an option when we want to be sure that copying and pickling are not involved and we get the same object as passed.

    2. Create an unique sentinel with unique unambiguous name when copy or unpickle a sentinel. This is a behavior of commonly used sentinels (object(), ['sentinel'], etc), and current behavior of mock.sentinel (except that the name is left the same and this is confusing).

    3. Preserve identity when copy or pickle/unpickle a sentinel. This is an option when test a function that uses multiprocessing or persistent cache and should restore an equivalent object. This is a behavior of enums, classes, functions or other global named objects.

    4. Preserve equality but not identity when copy or pickle/unpickle a sentinel. This is similar to the third option.

    All options look reasonable to me. I don't know what case is more common, what behavior is more expectable.

    @voidspace
    Copy link
    Contributor

    I think option 3 is the correct semantic behaviour for sentinels, and if there are already examples of this in the standard library then it *doesn't* violate expected behaviour of pickling and copying (sentinel and singleton objects can be permitted to retain this property through copying and pickling if it is a document facet of their behaviour and a natural result of their use cases).

    Sentinels exist *purely* to have unique, *identifiable* objects. So retaining identity is their "expected behaviour" as identity is their defining feature.

    So Serhiy, if you're happy that you've implemented this correctly - with tests and documentation updates (I see a separate doc patch) - go ahead and commit.

    @serhiy-storchaka
    Copy link
    Member

    sentinel-doc.patch documents the contrary behaviour -- not preserving an identity and equality.

    I haven't included doc changes because I don't know how to treat this change. As a bug fix, as a new feature, or as a new feature backported to maintained releases? If this is a new feature, it needs an entry in Whant's New and a versionchanged directive in the module documentation. If this is a bug fix, it likely doesn't need any special documenting.

    @voidspace
    Copy link
    Contributor

    It's a new feature.

    @serhiy-storchaka
    Copy link
    Member

    Updated patch contains doc changes.

    @serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jan 11, 2017
    @voidspace
    Copy link
    Contributor

    LGTM Serhiy - thank you for your work. Appreciated.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 11, 2017

    New changeset 98f061402fcf by Serhiy Storchaka in branch 'default':
    Issue bpo-20804: The unittest.mock.sentinel attributes now preserve their
    https://hg.python.org/cpython/rev/98f061402fcf

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 11, 2017

    New changeset 398b73dbb1a0 by Serhiy Storchaka in branch '3.5':
    Issue bpo-20804: Document the limitation of the unittest.mock.sentinel attributes.
    https://hg.python.org/cpython/rev/398b73dbb1a0

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your review Michael.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    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 tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants