classification
Title: Sentinels identity lost when pickled (unittest.mock)
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Vlastimil.Zíma, davin, michael.foord, peter.otten, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-02-28 09:22 by Vlastimil.Zíma, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
pickle_sentinels.py peter.otten, 2014-02-28 17:00
mock_sentinel_pickle.patch serhiy.storchaka, 2017-01-10 20:20 review
sentinel-doc.patch Vlastimil.Zíma, 2017-01-11 09:11
mock_sentinel_pickle2.patch serhiy.storchaka, 2017-01-11 17:03 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (28)
msg212417 - (view) Author: Vlastimil Zíma (Vlastimil.Zíma) Date: 2014-02-28 09:22
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.
msg212434 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-02-28 14:11
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.
msg212440 - (view) Author: Vlastimil Zíma (Vlastimil.Zíma) Date: 2014-02-28 15:03
I don't question pickle, but I'd expect sentinels to keep their equality when they are pickled or copied.
msg212451 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-02-28 15:59
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.
msg212452 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-02-28 16:01
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.
msg212454 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-02-28 16:10
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.
msg212455 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-02-28 16:14
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.
msg212460 - (view) Author: Peter Otten (peter.otten) * Date: 2014-02-28 17:00
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 ;)
msg212461 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-02-28 17:33
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.
msg212462 - (view) Author: Peter Otten (peter.otten) * Date: 2014-02-28 17:48
I have no experience with unittest.mock, so I'm in no position to contradict... Vlastimil, could you give your use case?
msg212476 - (view) Author: Vlastimil Zíma (Vlastimil.Zíma) Date: 2014-02-28 21:45
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.
msg212481 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-02-28 22:39
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).
msg212621 - (view) Author: Vlastimil Zíma (Vlastimil.Zíma) Date: 2014-03-03 09:24
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.
msg212633 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-03-03 12:28
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."
msg285152 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-10 20:20
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.
msg285156 - (view) Author: Davin Potts (davin) * (Python committer) Date: 2017-01-10 21:12
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.
msg285161 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-10 21:38
Sorry, I had wrote this patch for issue29229 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.
msg285202 - (view) Author: Vlastimil Zíma (Vlastimil.Zíma) Date: 2017-01-11 09:11
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
msg285203 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2017-01-11 09:15
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.
msg285208 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-11 10:27
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.
msg285212 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2017-01-11 10:52
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.
msg285239 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-11 16:44
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.
msg285241 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2017-01-11 16:46
It's a new feature.
msg285244 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-11 17:03
Updated patch contains doc changes.
msg285249 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2017-01-11 18:05
LGTM Serhiy - thank you for your work. Appreciated.
msg285251 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-11 18:13
New changeset 98f061402fcf by Serhiy Storchaka in branch 'default':
Issue #20804: The unittest.mock.sentinel attributes now preserve their
https://hg.python.org/cpython/rev/98f061402fcf
msg285252 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-11 18:18
New changeset 398b73dbb1a0 by Serhiy Storchaka in branch '3.5':
Issue #20804: Document the limitation of the unittest.mock.sentinel attributes.
https://hg.python.org/cpython/rev/398b73dbb1a0
msg285254 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-11 18:23
Thank you for your review Michael.
History
Date User Action Args
2017-03-31 16:36:24dstufftsetpull_requests: + pull_request979
2017-01-11 18:23:07serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg285254

stage: patch review -> resolved
2017-01-11 18:18:43python-devsetmessages: + msg285252
2017-01-11 18:13:57python-devsetnosy: + python-dev
messages: + msg285251
2017-01-11 18:05:21michael.foordsetmessages: + msg285249
2017-01-11 17:03:52serhiy.storchakasetfiles: + mock_sentinel_pickle2.patch
type: behavior -> enhancement
messages: + msg285244

versions: - Python 3.5, Python 3.6
2017-01-11 16:46:48michael.foordsetmessages: + msg285241
2017-01-11 16:44:37serhiy.storchakasetmessages: + msg285239
2017-01-11 10:52:06michael.foordsetmessages: + msg285212
2017-01-11 10:27:25serhiy.storchakasetmessages: + msg285208
2017-01-11 09:15:42michael.foordsetmessages: + msg285203
2017-01-11 09:11:58Vlastimil.Zímasetfiles: + sentinel-doc.patch

messages: + msg285202
2017-01-10 21:38:04serhiy.storchakasetmessages: + msg285161
2017-01-10 21:12:53davinsetnosy: + davin
messages: + msg285156
2017-01-10 20:20:06serhiy.storchakasetfiles: + mock_sentinel_pickle.patch

versions: + Python 3.5, Python 3.6, Python 3.7, - Python 3.3, Python 3.4
keywords: + patch
nosy: + serhiy.storchaka

messages: + msg285152
stage: patch review
2017-01-10 19:49:59davinlinkissue29229 superseder
2014-03-03 12:28:26michael.foordsetmessages: + msg212633
2014-03-03 09:24:23Vlastimil.Zímasetmessages: + msg212621
2014-02-28 22:39:37r.david.murraysetmessages: + msg212481
2014-02-28 21:45:24Vlastimil.Zímasetmessages: + msg212476
2014-02-28 17:48:33peter.ottensetmessages: + msg212462
2014-02-28 17:33:23r.david.murraysetmessages: + msg212461
2014-02-28 17:00:54peter.ottensetfiles: + pickle_sentinels.py
nosy: + peter.otten
messages: + msg212460

2014-02-28 16:14:27r.david.murraysetmessages: + msg212455
2014-02-28 16:10:14r.david.murraysetmessages: + msg212454
2014-02-28 16:01:50michael.foordsetmessages: + msg212452
2014-02-28 15:59:59r.david.murraysetmessages: + msg212451
2014-02-28 15:03:29Vlastimil.Zímasetmessages: + msg212440
2014-02-28 14:11:54r.david.murraysetnosy: + r.david.murray, michael.foord
messages: + msg212434
2014-02-28 09:22:37Vlastimil.Zímacreate