Skip to content

bpo-36102: Prepend slash to all POSIX shared memory block names #12036

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 25, 2019
Merged

bpo-36102: Prepend slash to all POSIX shared memory block names #12036

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33f2aee
Fix issue36102: Prepend slash to all POSIX shared memory block names …
applio Feb 25, 2019
6f513bd
Remove try-except as unnecessary.
applio Feb 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions Lib/multiprocessing/shared_memory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

# Shared memory block name prefix
if _USE_POSIX:
_SHM_NAME_PREFIX = 'psm_'
_SHM_NAME_PREFIX = '/psm_'
else:
_SHM_NAME_PREFIX = 'wnsm_'

Expand Down Expand Up @@ -68,6 +68,7 @@ class SharedMemory:
_buf = None
_flags = os.O_RDWR
_mode = 0o600
_prepend_leading_slash = True if _USE_POSIX else False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could simply be _prepend_leading_slash = _USE_POSIX (or you can simply avoid defining a class attribute for this)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By defining a class attribute, we can permit this to be turned off easily in a simple subclass.

class NoSlashesSharedMemory(SharedMemory):
    _prepend_leading_slash = False

Agreed that it could have simply been set equal to _USE_POSIX though I thought this would better emphasize that it is a True for POSIX and False otherwise. Perhaps this emphasis is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you (see my comment later).


def __init__(self, name=None, create=False, size=0):
if not size >= 0:
Expand Down Expand Up @@ -95,6 +96,7 @@ def __init__(self, name=None, create=False, size=0):
self._name = name
break
else:
name = "/" + name if self._prepend_leading_slash else name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider. This will make it impossible to attach to a "foo" literal (e.g. a memory name which was not created by this class). The _prepend_leading_slash class attribute can be toggled to allow this, therefore I think it should be a public attribute (not worth being documented).

Copy link
Contributor

@giampaolo giampaolo Feb 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm not sure if this use-case should be supported (and it looks pretty rare anyway). Having this as a settable class attribute is good already IMO. And if there's demand for it we can introduce it as a public property later. Dismiss my suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I was worried about needing to clarify that it only impacted POSIX systems and had no effect on Windows. At the same time, I saw value exposing it as a hook for users to control in a subclass (given that the leading slash is really only needed on FreeBSD/NetBSD and otherwise shortens the remaining space for the name). I thought to be cautious and start by making it private but maybe it should be made public?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it should remain private as per your point about Windows. If there ever will be demand for it we can simply add it as a prepend_leading_slash public property (with setter) so that we won't break code for those few people who in the meantime relied on _prepend_leading_slash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool -- sounds good.

self._fd = _posixshmem.shm_open(
name,
self._flags,
Expand Down Expand Up @@ -198,7 +200,11 @@ def buf(self):
@property
def name(self):
"Unique name that identifies the shared memory block."
return self._name
reported_name = self._name
if _USE_POSIX and self._prepend_leading_slash:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_USE_POSIX seems unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to parallel what was done in __init__() where the use of self._prepend_leading_slash only appears in the POSIX section and not at all in the Windows section of __init__(). If someone does subclass SharedMemory and chooses to set _prepend_leading_slash to True in that subclass as I suggested in my earlier comment, then when they try their code on Windows it would cause name() to behave oddly if we do not also have the check if _USE_POSIX ... here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

if self._name.startswith("/"):
reported_name = self._name[1:]
return reported_name

@property
def size(self):
Expand All @@ -224,8 +230,8 @@ def unlink(self):
In order to ensure proper cleanup of resources, unlink should be
called once (and only once) across all processes which have access
to the shared memory block."""
if _USE_POSIX and self.name:
_posixshmem.shm_unlink(self.name)
if _USE_POSIX and self._name:
_posixshmem.shm_unlink(self._name)


_encoding = "utf8"
Expand Down