This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

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

Created on 2021-09-07 10:17 by sobolevn, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 28294 merged sobolevn, 2021-09-11 19:11
PR 28675 merged miss-islington, 2021-10-01 10:47
Messages (5)
msg401219 - (view) Author: Nikita Sobolev (sobolevn) * (Python triager) 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.
msg403008 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-10-01 10:46
New changeset 746d648d47d12d16c2afedaeff626fc6aaaf6a46 by Nikita Sobolev in branch 'main':
bpo-45125: Improves pickling docs and tests for `shared_memory` (GH-28294)
https://github.com/python/cpython/commit/746d648d47d12d16c2afedaeff626fc6aaaf6a46
msg403013 - (view) Author: miss-islington (miss-islington) Date: 2021-10-01 13:09
New changeset fc3511f18e92ea345092aa59a4225218bd19e153 by Miss Islington (bot) in branch '3.10':
bpo-45125: Improves pickling docs and tests for `shared_memory` (GH-28294)
https://github.com/python/cpython/commit/fc3511f18e92ea345092aa59a4225218bd19e153
msg404725 - (view) Author: Nikita Sobolev (sobolevn) * (Python triager) Date: 2021-10-22 08:23
Thanks everyone. It is now fixed
History
Date User Action Args
2022-04-11 14:59:49adminsetgithub: 89288
2021-10-22 08:23:03sobolevnsetstatus: open -> closed
resolution: fixed
messages: + msg404725

stage: patch review -> resolved
2021-10-01 13:09:23miss-islingtonsetmessages: + msg403013
2021-10-01 10:47:10miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request27041
2021-10-01 10:46:06serhiy.storchakasetmessages: + msg403008
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