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

bpo-35813: Tests and docs for shared_memory #11816

Merged
merged 44 commits into from
Feb 24, 2019
Merged
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
720a8ea
Added tests for shared_memory submodule.
applio Feb 2, 2019
29a7f80
Added tests for ShareableList.
applio Feb 3, 2019
c56e29c
Fix bug in allocationn size during creation of empty ShareableList il…
applio Feb 3, 2019
c36de70
Initial set of docs for shared_memory module.
applio Feb 7, 2019
3c89c7c
Added docs for ShareableList, added doctree entry for shared_memory s…
applio Feb 8, 2019
5f4ba8f
Added examples to SharedMemoryManager docs, for ease of documentation…
applio Feb 9, 2019
f9aaa11
Wording tweaks to docs.
applio Feb 9, 2019
2377cfd
Fix test failures on Windows.
applio Feb 9, 2019
6bfa560
Added tests around SharedMemoryManager.
applio Feb 9, 2019
eaf7888
Documentation tweaks.
applio Feb 9, 2019
e166ed9
Fix inappropriate test on Windows.
applio Feb 9, 2019
0f18511
Further documentation tweaks.
applio Feb 11, 2019
a097dbb
Fix bare exception.
applio Feb 11, 2019
7c65017
Removed __copyright__.
applio Feb 11, 2019
da7731d
Fixed typo in doc, removed comment.
applio Feb 11, 2019
242a5e9
Merge remote-tracking branch 'upstream/master' into enh-tests-shmem
applio Feb 11, 2019
7bdfbbb
Updated SharedMemoryManager preliminary tests to reflect change of no…
applio Feb 11, 2019
eec4bb1
Added Sphinx doctest run controls.
applio Feb 11, 2019
1076567
CloseHandle should be in a finally block in case MapViewOfFile fails.
applio Feb 12, 2019
0be0531
Missed opportunity to use with statement.
applio Feb 12, 2019
1e5341e
Switch to self.addCleanup to spare long try/finally blocks and save o…
applio Feb 12, 2019
a5800a9
Simplify the posixshmem extension module.
nascheme Feb 13, 2019
34f1e9a
Added to doc around size parameter of SharedMemory.
applio Feb 16, 2019
9846290
Changed PosixSharedMemory.size to use os.fstat.
applio Feb 16, 2019
1f9bbf2
Change SharedMemory.buf to a read-only property as well as NamedShare…
applio Feb 17, 2019
69dd8a9
Marked as provisional per PEP411 in docstring.
applio Feb 17, 2019
8cf9ba3
Merge branch 'enh-tests-neilsimplify-shmem' into enh-tests-shmem
applio Feb 17, 2019
594140a
Changed SharedMemoryTracker to be private.
applio Feb 17, 2019
395709b
Removed registered Proxy Objects from SharedMemoryManager.
applio Feb 17, 2019
aa4a887
Removed shareable_wrap().
applio Feb 17, 2019
885592b
Removed shareable_wrap() and dangling references to it.
applio Feb 17, 2019
9001b76
Merge remote and local branches regarding elimination of
applio Feb 17, 2019
5848ec4
For consistency added __reduce__ to key classes.
applio Feb 17, 2019
6ff8eed
Fix for potential race condition on Windows for O_CREX.
applio Feb 18, 2019
06620e2
Remove unused imports.
applio Feb 18, 2019
868b83d
Update access to kernel32 on Windows per feedback from eryksun.
applio Feb 19, 2019
9d83b06
Moved kernel32 calls to _winapi.
applio Feb 20, 2019
715ded9
Removed ShareableList.copy as redundant.
applio Feb 20, 2019
6878533
Changes to _winapi use from eryksun feedback.
applio Feb 20, 2019
0d3d06f
Adopt simpler SharedMemory API, collapsing PosixSharedMemory and Wind…
applio Feb 21, 2019
05e26dd
Fix missing docstring on class, add test for ignoring size when attac…
applio Feb 21, 2019
7a3c7e5
Moved SharedMemoryManager to managers module, tweak to fragile test.
applio Feb 21, 2019
caf0a5d
Tweak to exception in OpenFileMapping suggested by eryksun.
applio Feb 21, 2019
12c097d
Mark a few dangling bits as private as suggested by Giampaolo.
applio Feb 22, 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
Prev Previous commit
Next Next commit
Change SharedMemory.buf to a read-only property as well as NamedShare…
…dMemory.size.
  • Loading branch information
applio committed Feb 17, 2019
commit 1f9bbf2fb8e482a625e0fc35af4c75faafa53f09
37 changes: 28 additions & 9 deletions Lib/multiprocessing/shared_memory.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,31 @@ def __init__(self, name, flags=None, mode=384, size=0, read_only=False):
raise FileExistsError(name)
Copy link
Contributor

@giampaolo giampaolo Feb 17, 2019

Choose a reason for hiding this comment

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

You could enrich the exception:

raise FileExistsError(errno.EEXIST, os.strerror(errno.EEXIST), name)

Result:

- FileExistsError: 'filename'
+ FileExistsError: [Errno 17] File exists: 'filename'

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 -- changed in 6878533.


self._mmap = mmap.mmap(-1, size, tagname=name)
Copy link
Contributor

@giampaolo giampaolo Feb 16, 2019

Choose a reason for hiding this comment

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

Same here (see my previous comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment for me too (ref the prior comment/conversation).

self.buf = memoryview(self._mmap)
self._buf = memoryview(self._mmap)
self.name = name
self.mode = mode
self.size = size
self._size = size

@property
def size(self):
"Size in bytes."
return self._size

@property
def buf(self):
"A memoryview of contents of the shared memory block."
return self._buf

def __repr__(self):
return f'{self.__class__.__name__}({self.name!r}, size={self.size})'

def close(self):
self.buf.release()
self._mmap.close()
if self._buf is not None:
self._buf.release()
self._buf = None
if self._mmap is not None:
self._mmap.close()
self._mmap = None

def unlink(self):
"""Windows ensures that destruction of the last reference to this
Expand Down Expand Up @@ -157,7 +171,7 @@ class PosixSharedMemory:
fd = -1
name = None
_mmap = None
buf = None
_buf = None

def __init__(self, name, flags=None, mode=384, size=0, read_only=False):
if name and (flags is None):
Expand Down Expand Up @@ -185,7 +199,7 @@ def __init__(self, name, flags=None, mode=384, size=0, read_only=False):
self.unlink()
raise
self._mmap = mmap.mmap(self.fd, self.size)
Copy link
Contributor

@giampaolo giampaolo Feb 16, 2019

Choose a reason for hiding this comment

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

This is not my territory, so take it with a grain of salt. I see that mmap.mmap provides some interesting arguments such as access, prot(ection), and flags:
https://docs.python.org/3/library/mmap.html#mmap.mmap
Depending on mode and read_only args that you provide here perhaps it would make sense to specify parameters for mmap in accordance (e.g. flags=MAP_SHARED, prot=PROT_WRITE/READ and access=ACCESS_READ/WRITE).
This is a private argument so maybe it's not so important. Just mentioning it for completeness/correctness.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a good point. With the simpler API introduced in commit 0d3d06f, we constrain the supported behaviors to no longer offer these sorts of control. The behavior of mmap seems to have some inconsistencies of its own across different platforms when trying to specify such modes of access -- I think this only encourages me to believe we have made the right choice with the simplified API.

self.buf = memoryview(self._mmap)
self._buf = memoryview(self._mmap)

@property
def size(self):
Expand All @@ -195,6 +209,11 @@ def size(self):
else:
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm... since you make a os.fstat call this may fail. And it's kinda unusual for a getattr to fail. Also, AFAIU the size may change if the memory is enlarged (it can happen I suppose). As such I think it may make sense to provide this as a method, not a property.

Copy link
Member Author

Choose a reason for hiding this comment

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

In commit 0d3d06f, I have eliminated the use of os.fstat() in the property for size. Instead, it is now an unchanging value stored as part of the __init__().


@property
def buf(self):
"A memoryview of contents of the shared memory block."
return self._buf

def _open_retry(self):
# generate a random name, open, retry if it exists
while True:
Expand All @@ -215,9 +234,9 @@ def unlink(self):
_posixshmem.shm_unlink(self.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't unlink also call close?

Copy link
Member Author

@applio applio Feb 17, 2019

Choose a reason for hiding this comment

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

In the same sense that close() should not call unlink(), the reverse should also be true to maintain a clean separation of duties. This keeps things clear in situations like the following: if the coordinating process is done accessing the shared memory block, it should be permitted to call close at that time and wait to call unlink once it knows all other processes are similarly done; in this case we would not want unlink to call close a second time. After all, if unlink did call close, it would only be able to do so in one process but other processes should still call close. Best to keep them separated. There is a reason why this is the design for the shared memory C APIs that we use on various operating systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clear enough. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a second thought... if unlink() is called, isn't the instance supposed to be unusable? AFAICT you can still play with the memoryview object but only locally (no data is shared). Also, I wonder whether it makes sense to protect buf attribute so that it raises an exception if used after close() and unlink().

Copy link
Member Author

Choose a reason for hiding this comment

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

If one process calls unlink(), we might protect buf and even do all the other things that close() would do for that one process but all other processes still need to call close() themselves. I think the clean separation of duties is still the right answer here.

The docs currently say that if unlink() is called, any continued use is unsupported. The exact behavior of unlink with respect to when the shared memory block truly is destroyed is not something we can ultimately control or enforce. Users must call close() and unlink() to ensure proper cleanup. If users can not follow this directive, I doubt we could ever truly protect them from themselves.


def close(self):
if self.buf is not None:
self.buf.release()
self.buf = None
if self._buf is not None:
self._buf.release()
self._buf = None
if self._mmap is not None:
self._mmap.close()
self._mmap = None
Expand Down