bpo-36102: Prepend slash to all POSIX shared memory block names #12036
Conversation
applio
changed the title
| @@ -68,6 +68,7 @@ class SharedMemory: | |||
| _buf = None | |||
| _flags = os.O_RDWR | |||
| _mode = 0o600 | |||
| _prepend_leading_slash = True if _USE_POSIX else False | |||
There was a problem hiding this comment.
This could simply be _prepend_leading_slash = _USE_POSIX (or you can simply avoid defining a class attribute for this)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree with you (see my comment later).
Lib/multiprocessing/shared_memory.py
Outdated
| try: | ||
| if self._name[0] == "/": | ||
| reported_name = self._name[1:] | ||
| except Exception: |
There was a problem hiding this comment.
Looks like catching exception is unnecessary. Also consider using self._name.startswith("/").
There was a problem hiding this comment.
My concern was the possibility that self._name could be set to something other than a str (especially if it were None). Granted, I have not tested using bytes for the name, but it did cross my mind that it might work, in which case self._name[0] == "/" would still work but self._name.startswith("/") would complain that it was not passed b"/".
There was a problem hiding this comment.
The C module only accepts strings so you'll fail in __init__ (where _name is always set). Don't worry about nasty things users may do with it (not your responsability)
There was a problem hiding this comment.
Ah, good. I have removed the try-except and changed to use str.startswith in commit 6f513bd.
I almost can't help but think about nasty things users may do.... the horror... =)
| @@ -198,7 +200,14 @@ 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: | |||
There was a problem hiding this comment.
_USE_POSIX seems unnecessary
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
When you're done making the requested changes, leave the comment: |
|
@applio: Please replace |
Prepend slash to all POSIX shared memory block names by default to avoid needing to add special handling for systems (i.e. FreeBSD) that require it.
https://bugs.python.org/issue36102