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

Jump to bottom

Improve tests and docs of how pickle works with SharedMemory obejcts #89288

Closed
sobolevn opened this issue Sep 7, 2021 · 5 comments
Closed

Improve tests and docs of how pickle works with SharedMemory obejcts #89288

sobolevn opened this issue Sep 7, 2021 · 5 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes docs Documentation in the Doc dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@sobolevn
Copy link
Member

sobolevn commented Sep 7, 2021

BPO 45125
Nosy @serhiy-storchaka, @miss-islington, @sobolevn
PRs
  • bpo-45125: Improves pickling docs and tests for shared_memory #28294
  • [3.10] bpo-45125: Improves pickling docs and tests for shared_memory (GH-28294) #28675
    		•

    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 2021-10-22.08:23:03.947>
created_at = <Date 2021-09-07.10:17:52.925>
labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', 'tests', 'docs']
title = 'Improve tests and docs of how `pickle` works with `SharedMemory` obejcts'
updated_at = <Date 2021-10-22.08:23:03.946>
user = 'https://github.com/sobolevn'

    bugs.python.org fields:

    activity = <Date 2021-10-22.08:23:03.946>
actor = 'sobolevn'
assignee = 'docs@python'
closed = True
closed_date = <Date 2021-10-22.08:23:03.947>
closer = 'sobolevn'
components = ['Documentation', 'Tests']
creation = <Date 2021-09-07.10:17:52.925>
creator = 'sobolevn'
dependencies = []
files = []
hgrepos = []
issue_num = 45125
keywords = ['patch']
message_count = 5.0
messages = ['401219', '401229', '403008', '403013', '404725']
nosy_count = 4.0
nosy_names = ['docs@python', 'serhiy.storchaka', 'miss-islington', 'sobolevn']
pr_nums = ['28294', '28675']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue45125'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']
    @sobolevn
    Copy link
    Member Author

    sobolevn commented Sep 7, 2021

    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 (

    cpython/Lib/test/_test_multiprocessing.py

    Lines 3789 to 3794 in a5c6bcf

    # Test pickling
    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')
    ):

    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 (

    cpython/Lib/test/_test_multiprocessing.py

    Lines 4121 to 4144 in a5c6bcf

    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()
    ):

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

    @sobolevn sobolevn added 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Sep 7, 2021
    @sobolevn sobolevn added docs Documentation in the Doc dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Sep 7, 2021
    @sobolevn sobolevn added docs Documentation in the Doc dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Sep 7, 2021
    @serhiy-storchaka
    Copy link
    Member

    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.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 746d648 by Nikita Sobolev in branch 'main':
    bpo-45125: Improves pickling docs and tests for shared_memory (GH-28294)
    746d648

    @miss-islington
    Copy link
    Contributor

    New changeset fc3511f by Miss Islington (bot) in branch '3.10':
    bpo-45125: Improves pickling docs and tests for shared_memory (GH-28294)
    fc3511f

    @sobolevn
    Copy link
    Member Author

    Thanks everyone. It is now fixed

    @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
    Assignees
    No one assigned
    Labels
    3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes docs Documentation in the Doc dir tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @serhiy-storchaka @sobolevn @miss-islington