classification
Title: Improve tests and docs of how `pickle` works with `SharedMemory` obejcts
Type: behavior Stage: patch review
Components: Documentation, Tests Versions: Python 3.11, Python 3.10, Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: docs@python Nosy List: docs@python, serhiy.storchaka, sobolevn
Priority: normal Keywords: patch

Created on 2021-09-07 10:17 by sobolevn, last changed 2021-09-11 19:11 by sobolevn.

Pull Requests
URL Status Linked Edit
PR 28294 open sobolevn, 2021-09-11 19:11
Messages (2)
msg401219 - (view) Author: Nikita Sobolev (sobolevn) * Date: 2021-09-07 10:17
As we've discussed in https://bugs.python.org/msg401146 we need to improve how `SharedMemory` is documented and tested. Right now docs do not cover `pickle` at all (https://github.com/python/cpython/blob/main/Doc/library/multiprocessing.shared_memory.rst). And this is an important aspect in multiprocessing.

`SharedMemory` and `pickle` in `test_shared_memory_basics` (https://github.com/python/cpython/blob/a5c6bcf24479934fe9c5b859dd1cf72685a0003a/Lib/test/_test_multiprocessing.py#L3789-L3794):

```
sms.buf[0:6] = b'pickle'
pickled_sms = pickle.dumps(sms)
sms2 = pickle.loads(pickled_sms)
self.assertEqual(sms.name, sms2.name)
self.assertEqual(bytes(sms.buf[0:6]), bytes(sms2.buf[0:6]), b'pickle')
```

`ShareableList` has better coverage in this regard (https://github.com/python/cpython/blob/a5c6bcf24479934fe9c5b859dd1cf72685a0003a/Lib/test/_test_multiprocessing.py#L4121-L4144):

```
    def test_shared_memory_ShareableList_pickling(self):
        sl = shared_memory.ShareableList(range(10))
        self.addCleanup(sl.shm.unlink)

        serialized_sl = pickle.dumps(sl)
        deserialized_sl = pickle.loads(serialized_sl)
        self.assertTrue(
            isinstance(deserialized_sl, shared_memory.ShareableList)
        )
        self.assertTrue(deserialized_sl[-1], 9)
        self.assertFalse(sl is deserialized_sl)
        deserialized_sl[4] = "changed"
        self.assertEqual(sl[4], "changed")

        # Verify data is not being put into the pickled representation.
        name = 'a' * len(sl.shm.name)
        larger_sl = shared_memory.ShareableList(range(400))
        self.addCleanup(larger_sl.shm.unlink)
        serialized_larger_sl = pickle.dumps(larger_sl)
        self.assertTrue(len(serialized_sl) == len(serialized_larger_sl))
        larger_sl.shm.close()

        deserialized_sl.shm.close()
        sl.shm.close()
```

So, my plan is:
1. Improve testing of `SharedMemory` after pickling/unpickling. I will create a separate test with all the `pickle`-related stuff there
2. Improve docs: user must understand what will have when `SharedMemory`  / `SharableList` is pickled and unpickled. For example, the fact that it will still be shared can be a surprise to many.

I am going to send a PR with both thing somewhere this week.

I will glad to head any feedback before / after :)
msg401229 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-09-07 10:36
Go ahead. Fix also other issues in tests:

* All pickling tests should test with all supported protocols, not just with the default one.

* self.assertTrue(
            isinstance(deserialized_sl, shared_memory.ShareableList)
        )
assertIsinstance() can be used here.

* self.assertTrue(deserialized_sl[-1], 9)
What is this? If it tests that deserialized_sl[-1] is true, 9 should be removed. Should it be assertEqual() instead?

* self.assertFalse(sl is deserialized_sl)
assertIs() can be used here.

* self.assertTrue(len(serialized_sl) == len(serialized_larger_sl))
assertEqual() can be used here.

* All close() should either be called in the finally block, or registered with addCleanup(). Otherwise we will heave resource leaks if tests fail.

There may be other issues in other tests. Seems these tests need a clean up.
History
Date User Action Args
2021-09-11 19:11:30sobolevnsetkeywords: + patch
stage: patch review
pull_requests: + pull_request26710
2021-09-07 10:36:08serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg401229
2021-09-07 10:17:52sobolevncreate