bpo-35813: Added shared_memory submodule of multiprocessing. by applio · Pull Request #11664 · python/cpython

applio · 2019-01-24T04:09:13Z

Tests and docs are still absent. Some cleanup remains to be done in the code as well but all appears to be functional and stable across platforms since the sprint in September 2018.

https://bugs.python.org/issue35813

…ng, fixed pep8 details.

vpatov · 2019-01-27T07:18:12Z

Lib/multiprocessing/shared_memory.py

+
+    def __init__(self, name, flags=None, mode=None, size=None, read_only=False):
+        if name is None:
+            name = f'wnsm_{os.getpid()}_{random.randrange(100000)}'


random.randrange(100000)
Why use a random number here?

If the user does not supply a name for the shared memory segment, an attempt is made to create a hopefully unique name. There are system-imposed constraints on the length of the name used and (I believe) the use of only ascii characters in the name but the use of a number here is not a requirement. Are you thinking about suggesting a mixture of letters and numbers?
An accidental collision with an existing shared memory segment's name would likely result in unintended/undesirable behavior.

Wouldn't using a UUID give better uniqueness here, or just using itertools.count() instead of a random renumber to number memory segments? The latter avoids conflicts in a single process, although there still might be conflicts between processes when old segments aren't cleaned up properly.

Replaced by Neil's patch with a stronger solution in GH-11816.

…darwin.

… _Server.

bedevere-bot · 2019-02-02T04:52:25Z

@applio: Please replace # with GH- in the commit message next time. Thanks!

bedevere-bot · 2019-02-02T05:22:38Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD 10-STABLE Non-Debug 3.x has failed when building commit e5ef45b.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/167/builds/531) and take a look at the build logs.
Check if the failure is related to this commit (e5ef45b) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/167/builds/531

Click to see traceback logs

Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/test/_test_multiprocessing.py", line 1866, in test_default_timeout
    self.assertEqual(len(results), barrier.parties)
AssertionError: 0 != 5

----------------------------------------------------------------------

Ran 324 tests in 348.271s

FAILED (failures=1, skipped=32)

ronaldoussoren

I haven't reviewed most of the Python code, due to lack of documentation it is unclear what the intended behaviour of of the code is.

ronaldoussoren · 2019-02-03T09:59:20Z

Lib/multiprocessing/shared_memory.py

+
+    def __init__(self, name, flags=None, mode=None, size=None, read_only=False):
+        if name is None:
+            name = f'wnsm_{os.getpid()}_{random.randrange(100000)}'


Wouldn't using a UUID give better uniqueness here, or just using itertools.count() instead of a random renumber to number memory segments? The latter avoids conflicts in a single process, although there still might be conflicts between processes when old segments aren't cleaned up properly.

ronaldoussoren · 2019-02-03T10:00:47Z

Lib/multiprocessing/shared_memory.py

+        pass
+
+
+class PosixSharedMemory(_PosixSharedMemory):


Why is this class defined unconditionally when it relies on functionality from an optional C extension. I'd leave the class out entirely when running on platform that doesn't support the C extension because that makes it clearer that functionality is not present.

Removed in GH-11816.

ronaldoussoren · 2019-02-03T10:02:51Z

Lib/multiprocessing/shared_memory.py

+        self.close_fd()
+
+
+class SharedMemory:


I personally don't like this pattern. This is basically a function call, and could also just be an alias. Something like:

if os.name == 'nt': SharedMemory = WindowsSharedMemory else: SharedMemory = PosixSharedMemory

Removed in GH-11816.

ronaldoussoren · 2019-02-03T10:06:25Z

Lib/multiprocessing/shared_memory.py

+            )
+            return f"{self.__class__.__name__}({', '.join(formatted_pairs)})"
+
+        #def __getstate__(self):


This is commented-out code and should be removed.

Removed in GH-11816.

ronaldoussoren · 2019-02-03T10:08:26Z

Lib/multiprocessing/shared_memory.py

+    packing format for any storable value must require no more than 8
+    characters to describe its format."""
+
+    # TODO: Adjust for discovered word size of machine.


Please actually adjust for different word sizes, if needed (another option is to require that the size of the C double is 8 bytes and that a 64-bit integer type is used for integers).

Comment was moot and now removed in GH-11816.

ronaldoussoren · 2019-02-03T10:32:06Z

Modules/_multiprocessing/posixshmem.c

+    PyObject *module;
+    PyObject *module_dict;
+
+    // I call this in case I'm asked to create any random names.


This changes process global state. Please remove.

Furthermore creating random names in C code isn't needed anyway (see one of the earlier comments)

Removed in GH-11816.

ronaldoussoren · 2019-02-03T10:34:45Z

Modules/_multiprocessing/posixshmem.c

+    PyModule_AddObject(module, "_PosixSharedMemory", (PyObject *)&SharedMemoryType);
+
+
+    PyModule_AddStringConstant(module, "__copyright__", "Copyright 2012 Philip Semanchuk, 2018-2019 Davin Potts");


The only other files with a copyright attribute are parser, optparse and platform. I'd prefer to avoid adding new copyright attributes to stdlib modules.

See comment on issue.

Removed in 7c65017 which should appear in GH-11816 shortly.

I have pinged Van again asking for verification that this is indeed appropriate but I have made the change in anticipation that this is all good.

ronaldoussoren · 2019-02-03T10:36:21Z

Modules/_multiprocessing/posixshmem.c

+    PyModule_AddIntConstant(module, "O_EXCL", O_EXCL);
+    PyModule_AddIntConstant(module, "O_CREX", O_CREAT | O_EXCL);
+    PyModule_AddIntConstant(module, "O_TRUNC", O_TRUNC);
+


These values are already attributes of the os module, don't add them to this module as well.

Removed in GH-11816.

ronaldoussoren · 2019-02-03T10:37:01Z

Modules/_multiprocessing/posixshmem.c

+    else
+        PyDict_SetItemString(module_dict, "Error", pBaseException);
+
+    if (!(pPermissionsException = PyErr_NewException("_posixshmem.PermissionsError", pBaseException, NULL)))


Why not use PermissionsError instead of a specific error?

+1 to using PermissionsError. I used a custom error in the original code for Python 2.x support.

Changed in GH-11816.

ronaldoussoren · 2019-02-03T10:37:32Z

Modules/_multiprocessing/posixshmem.c

+    else
+        PyDict_SetItemString(module_dict, "PermissionsError", pPermissionsException);
+
+    if (!(pExistentialException = PyErr_NewException("_posixshmem.ExistentialError", pBaseException, NULL)))


Why not use FileNotFoundError instead of a specific exception?

Changed in GH-11816.

giampaolo · 2019-02-05T15:31:47Z

Lib/multiprocessing/shared_memory.py

+                assert hasattr(self, "_proxied_type")
+            try:
+                existing_type.__init__(self, *args, **kwargs)
+            except:


bare except clause should be avoided (use except Exception: instead)

Agreed! I have changed this in a097dbb as part of PR-11816.

Added shared_memory submodule of multiprocessing.

90f4a6c

the-knights-who-say-ni added the CLA signed label Jan 24, 2019

bedevere-bot added the awaiting merge label Jan 24, 2019

applio added 3 commits January 23, 2019 22:45

Fixed as yet unsupported expression, added news entry.

fa42acf

Formatting tweaks.

fdb694c

Removed unfavored alt_shareable_wrap approach, cleaned up debug loggi…

9cbdac8

…ng, fixed pep8 details.

vpatov reviewed Jan 27, 2019

View reviewed changes

applio added 4 commits January 31, 2019 22:17

Fixed missing library dependency on linux and bsd systems other than …

a1070ac

…darwin.

Fixed whitespace.

fac0895

Added missing __all__, ensured SharedMemoryManager uses parameterized…

c2ef47d

… _Server.

Adjust for build on OpenBSD and others.

516cf4a

applio merged commit e5ef45b into python:master Feb 2, 2019

bedevere-bot removed the awaiting merge label Feb 2, 2019

ronaldoussoren reviewed Feb 3, 2019

View reviewed changes

giampaolo mentioned this pull request Feb 6, 2019

bpo-35917: Test multiprocessing manager classes and shareable types #11772

Merged

giampaolo reviewed Feb 7, 2019

View reviewed changes

eendebakpt mentioned this pull request Nov 28, 2022

ShareableList removes trailing zeroes from strings and bytes #96779

Open

applio mentioned this pull request May 11, 2023

shared memory construct to avoid need for serialization between processes #79994

Closed

		PyModule_AddObject(module, "_PosixSharedMemory", (PyObject *)&SharedMemoryType);


		PyModule_AddStringConstant(module, "__copyright__", "Copyright 2012 Philip Semanchuk, 2018-2019 Davin Potts");

Uh oh!

Conversation

applio commented Jan 24, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 2, 2019

Uh oh!

bedevere-bot commented Feb 2, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

ronaldoussoren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

applio commented Jan 24, 2019 •

edited by bedevere-bot

Loading