bpo-35813: Tests and docs for shared_memory #11816

applio · 2019-02-11T04:39:47Z

Tests and docs are now present. Discussion around the API for the SharedMemory class and the appropriateness of the shareable_wrap() function is requested in particular.

https://bugs.python.org/issue35813

…luminated by existing test run on Linux.

…ubmodule, name refactoring for greater clarity.

… switched away from exclusively registered functions to some explicit methods on SharedMemoryManager.

eamanu

Good job!!! Please consider review my comments. Please review the CI problems.

eamanu · 2019-02-11T13:19:33Z

Doc/library/multiprocessing.shared_memory.rst

+
+This module provides a class, :class:`SharedMemory`, for the allocation
+and management of shared memory to be accessed by one or more processes
+on a multicore or symmetric multiprocessor (SMP) machine.  To assist with


the double whitepace on machine. To assist if for more readable?

In the resulting html, whether one or two spaces are in the source only one space is presented in the browser. The convention of using two spaces predates patterns of using only one space after a period. As far as I know, both are "correct". Thankfully the output is the same either way.

I did not go looking for this but I accidentally stumbled across it in the dev guide (see devguide.python.org for lots more such goodies):

A sentence-ending period may be followed by one or two spaces; while reST ignores the second space, it is customarily put in by some users, for example to aid Emacs’ auto-fill mode.

Oh great thanks for the clarification. I had the idea that double space is used just on docstring.

eamanu · 2019-02-11T14:08:04Z

Doc/library/multiprocessing.shared_memory.rst

+
+   >>> from multiprocessing import shared_memory
+   >>> shm_a = shared_memory.SharedMemory(None, size=10)
+   >>> type(shm_b.buf)


Where do you define shm_b?

Great spotting that missed detail! Fixed in commit da7731d.

eamanu · 2019-02-11T14:09:08Z

Doc/library/multiprocessing.shared_memory.rst

+   >>> len(buffer)
+   10
+   >>> buffer[:4] = bytearray([22, 33, 44, 55])  # Modify multiple at once
+   >>> buffer[4] = 100                           # Modify single byte at a time


I don't know if pep8 will fail here. Because it say you that you need have two whitespaces.

I believe this physical alignment of two related comments helps readability significantly. To not do so would make the example much less readable. Pep8 encourages considering this in particular.

eamanu · 2019-02-11T14:09:31Z

Doc/library/multiprocessing.shared_memory.rst

+   >>> array.array('b', shm_b.buf[:5])  # Copy the data into a new array.array
+   array('b', [22, 33, 44, 55, 100])
+   >>> shm_b.buf[:5] = b'howdy'  # Modify via shm_b using bytes
+   >>> bytes(shm_a.buf[:5])      # Access via shm_a


Same comment as on the prior; I believe this significantly helps readability in a situation where such help is needed.

eamanu · 2019-02-11T14:10:29Z

Doc/library/multiprocessing.shared_memory.rst

+
+
+The following example demonstrates a practical use of the :class:`SharedMemory`
+class with `NumPy arrays <https://www.numpy.org/>`_, accessing the


I don't know if is necessary show a sample with NumPy, it's just an opinion

The use of NumPy arrays with shared memory is anticipated to be one of the most popular use cases for working with shared memory. Demonstrating this combination is likely very important to a large number of people.

eamanu · 2019-02-11T14:11:00Z

Doc/library/multiprocessing.shared_memory.rst

+
+   A call to :meth:`~multiprocessing.managers.BaseManager.start` on a
+   :class:`SharedMemoryManager` instance causes a new process to be started.
+   This new process's sole purpose is to manage the life cycle


process' ?

After double-checking with a technical editor just now, I have been assured that either is correct.

eamanu · 2019-02-11T14:15:39Z

Lib/multiprocessing/shared_memory.py

@@ -8,34 +8,120 @@

 from functools import reduce
 import mmap
-from .managers import DictProxy, SyncManager, Server
+from .managers import dispatch, BaseManager, Server, State, ProcessError, \


What do you think about use () on imports? This way you avoid use \

I like both styles. At the moment, I think it best to stay consistent with the pattern that already exists in multiprocessing and its submodules.

Lib/multiprocessing/shared_memory.py

…t supporting all registered functions on SyncManager.

giampaolo · 2019-02-11T20:49:55Z

Doc/library/multiprocessing.shared_memory.rst

+   *name* is the unique name for the requested shared memory, specified as
+   a string.  If ``None`` is supplied for the name, a new shared memory
+   block with a novel name will be created without needing to also specify
+   ``flags``.


Since it appears name is optional, why not make it a kwarg?

Moved this topic to b.p.o. starting with msg335660 there.

giampaolo · 2019-02-11T20:52:05Z

Doc/library/multiprocessing.shared_memory.rst

+   exception will be raised.  To request the creation of a new shared
+   memory block, set to ``O_CREX``.  To request the optional creation of a
+   new shared memory block or attach to an existing one by the same name,
+   set to ``O_CREAT``.


It appears deciding whether to use O_CREX / O_CREAT can be implemented as an internal detail based on whether the name is provided or not. Are there other use cases for flags argument?

Moved this topic to b.p.o. starting with msg335660 there.

giampaolo · 2019-02-11T20:55:05Z

Doc/library/multiprocessing.shared_memory.rst

+   set to ``O_CREAT``.
+
+   *mode* controls user/group/all-based read/write permissions on the
+   shared memory block.  Its specification is not enforceable on all platforms.


Is mode intended to work like the mode of UNIX file? (I see it defaults to 0o600 so it appears it does). What is the use case for it?

Moved this topic to b.p.o. starting with msg335660 there.

giampaolo · 2019-02-11T21:16:32Z

Doc/library/multiprocessing.shared_memory.rst

+copying of data.
+
+
+.. class:: SharedMemory(name, flags=None, mode=0o600, size=0, read_only=False)


If I understand your code samples it seems this class/object can be returned via the SharedMemoryManager as in:

>>> mgr = SharedMemoryManager() >>> mgr.start() >>> raw_shm = smm.SharedMemory(size=128)

As such, what's the use case for using SharedMemory directly? Also, what's the use case for the name parameter? I understand it you can use it for attaching to a pre-existing instance of SharedMemory, but this kind of diverges from the base SyncManager which tipically allows sharing objects by passing them as arguments for the worker. E.g., with a plain dict:

import multiprocessing import multiprocessing.managers def worker(d): print(d) # prints {'foo': 1} d['bar'] = 2 # I can also change the dict from here (child process) manager = multiprocessing.managers.SyncManager() manager.start() d = manager.dict() d['foo'] = 1 proc = multiprocessing.Process(target=worker, args=(d, )) proc.start() proc.join()

I wonder if this (the name) can be made transparent (aka an implementation detail).

Moved this topic to b.p.o. starting with msg335622 there.

giampaolo · 2019-02-11T21:21:24Z

Doc/library/multiprocessing.shared_memory.rst

+
+   *read_only* controls whether a shared memory block is to be available
+   for only reading or for both reading and writing.  Its specification is
+   not enforceable on all platforms.


You should mention what platforms implement this and what happens when it's not and True is passed. Also, since this appears not portable across platform, perhaps it's better to not support it at all.

Agreed -- with the simpler API introduced in commit 0d3d06f, we no longer expose users to such complications.

giampaolo · 2019-02-11T22:07:50Z

Lib/test/_test_multiprocessing.py

+
+    def setUp(self):
+        if not HAS_SHMEM:
+            self.skipTest("requires multiprocessing.shared_memory")


You can decorate the class instead

Most of the other tests in _test_multiprocessing.py use this (potentially older) style rather than the decorator, but this is no longer universally true. I will adopt the decorator style for new tests added to the testing module. Changed in commit 1e5341e as well.

giampaolo · 2019-02-11T22:08:51Z

Lib/test/_test_multiprocessing.py

+
+        finally:
+            # Prevent test failures from leading to a dangling segment.
+            sms.unlink()


You could use self.addCleanup(sms.unlink) right after creating sms, spare this long try/finally block and save one indentation.

Good thought. Changed in commit 1e5341e.

giampaolo · 2019-02-11T22:08:57Z

Lib/test/_test_multiprocessing.py

+            sms.close()
+
+        finally:
+            sms.unlink()


You could use self.addCleanup(sms.unlink) right after creating sms, spare this long try/finally block and save one indentation.

Good thought -- same as prior comment.

giampaolo · 2019-02-11T22:09:49Z

Lib/test/_test_multiprocessing.py

+                        sms_uno.close()
+
+                    finally:
+                        sms_uno.unlink()  # A second shm_unlink() call is bad.


also here (could use addCleanup)

Actually, this one needs to stay as it is, because this second call to unlink() is needed to trigger the exception that we are testing for.

giampaolo · 2019-02-11T22:15:57Z

Lib/test/_test_multiprocessing.py

+
+    def test_joinable_queue(self):
+        self.test_queue("JoinableQueue")
+


I don't understand why you need to move these 2 methods from the previous tests. The logic appears split between the mixing class and this one for no apparent reason (but I may be missing it). If you need a different logic for certain TestSharedMemoryManagerTypes methods/tests you can simply override them (e.g. if you want to skip them).

Both Queue and JoinableQueue are implemented as distributed shared memory (a queue.Queue object existing in one process and an AutoProxy object existing in the local process). For the same reasons mentioned in my b.p.o. comment, I believe it is much clearer to not include these on SharedMemoryManager by default and to instead document how they may be added on-demand with a single line of code (by calling register()).

There is also the potential for a POSIX shared memory message queue (not currently implemented in shared_memory.py). This is also something worth considering for the future.

Question: if we expose them they are not subject to the speedup, correct? Would it be the same as using them via the SyncManager?

Correct, exposing Queue or JoinableQueue here would not make them faster; they would be the same as if using them via SyncManager.

We could simply encourage users to employ a SyncManager anytime they want to use a Queue or Lock but this forces the creation of two managers with a server behind each and therefore two processes instead of only one. I think it will be easier to explain, "create one manager to oversee work and provide cleanup at the end."

Although my use of empty() in this example is admittedly fragile, this example shows augmenting SyncManager with Queue is really only one line of code:

from multiprocessing import shared_memory import queue shared_memory.SharedMemoryManager.register('Queue', queue.Queue) with shared_memory.SharedMemoryManager() as smm: sl = smm.ShareableList("feb") # fast, zero-copy shared memory q = smm.Queue() # slow, copy+message distributed shared memory for letter in sl: q.put(letter.upper()) while not q.empty(): print(q.get(timeout=1))

I also think we should get rid of Queue, Lock, etc. Just for confirmation: ShareableList and AsyncManager.Lock can be safely mixed/used together, correct?

Agreed -- I have now gotten rid of Queue, Lock, etc. in commit 395709b.

Yes, confirmed: ShareableList and SyncManager.Lock can be safely mixed/used together.

I was sure you meant SyncManager when your wrote AsyncManager but I still went searching just to make sure there was not some AsyncManager hiding inside asyncio somewhere that I was not thinking of. =)

bedevere-bot · 2019-02-11T22:51:45Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

…ne indentation, change to use decorator to skip test instead.

osvenskan

Nice simplifications! This looks good to me.

eryksun · 2019-02-19T05:01:25Z

Lib/multiprocessing/shared_memory.py

+    import ctypes
+    from ctypes import wintypes
+
+    kernel32 = ctypes.windll.kernel32


This should be kernel32 = ctypes.WinDLL('kernel32', use_last_error=True). With ctypes.windll, it's using a globally cached WinDLL instance. This in turn caches function pointers, which leads to conflicting prototypes between packages. Also, use_last_error is required for ctypes.get_last_error(), which is more reliable than FFI-based ctypes.GetLastError().

Is this ctypes usage just temporary for experimenting until the API is finalized? Ultimately I hope this uses _winapi instead of ctypes.

Thanks, eryksun -- you rock. Changed both in commit 868b83d.

I believe the API is close enough to being finalized that we can move this to _winapi now. I would very much appreciate your help reviewing that move, if I can talk you into that.

I have moved this over to use _winapi now, changed in commit 9d83b06.

If you are up for helping review this too, it would again be very much appreciated.

eryksun · 2019-02-19T05:04:32Z

Lib/multiprocessing/shared_memory.py

+                name
+            )
+            try:
+                last_error_code = kernel32.GetLastError()


This should call ctypes.get_last_error(), which is being called elsewhere. But for it to work, we need use_last_error=True, as mentioned above.

Strike that. kernel32.CreateFileMappingW._errcheck is set to _errcheck_bool, which should raise FileExistsError here. Maybe this was added due to a misunderstanding. Using ctypes.windll.kernel32 renders the _errcheck function useless, since ctypes.get_last_error isn't enabled.

When kernel32.CreateFileMappingW is passed the name of an existing anonymous memory block in this way, it succeeds and it does not raise FileExistsError (_errcheck_bool receives a non-zero result value). This motivates the ctypes.get_last_error() call to check for a return value of 183.

This seems to match what is described on docs.microsoft.com, regarding the return value from CreateFileMappingA:

If the object exists before the function call, the function returns a handle to the existing object (with its current size, not the specified size), and GetLastError returns ERROR_ALREADY_EXISTS.

FWIW, this technique is leveraged in the Boost library for this same purpose. The following is a snippet from boost/interprocess/windows_shared_memory.hpp where create_file_mapping is a wrapper for CreateFileMappingW:

switch(type){ case ipcdetail::DoOpen: m_handle = winapi::open_file_mapping(map_access, filename); break; case ipcdetail::DoCreate: case ipcdetail::DoOpenOrCreate: { m_handle = winapi::create_file_mapping ( winapi::invalid_handle_value, protection, size, filename , (winapi::interprocess_security_attributes*)perm.get_permissions()); } break; default: { error_info err = other_error; throw interprocess_exception(err); } } if(!m_handle || (type == ipcdetail::DoCreate && winapi::get_last_error() == winapi::error_already_exists)){ error_info err = system_error_code(); this->priv_close(); throw interprocess_exception(err); }

I would be happier if CreateFileMappingW behaved as you suggested it might.

it succeeds and it does not raise FileExistsError (_errcheck_bool receives a non-zero result value). This motivates the ctypes.get_last_error() call to check for a return value of 183.

Sorry it slipped my mind that Windows normally uses the OBJ_OPENIF disposition when creating a named kernel object. If this flag weren't used, then the underlying NtCreateSection system call would fail with STATUS_OBJECT_NAME_COLLISION, which maps to Windows ERROR_ALREADY_EXISTS. Instead it succeeds, and CreateFileMapping manually sets the last error. It's one of a small number of cases in which a WINAPI call succeeds with an error set.

eryksun · 2019-02-19T05:19:25Z

Lib/multiprocessing/shared_memory.py

+    PAGE_READONLY = 0x02
+    PAGE_EXECUTE_READWRITE = 0x04
+    INVALID_HANDLE_VALUE = -1
+    FILE_ALREADY_EXISTS = 183


This constant shouldn't be necessary once other mistakes are corrected. But it should be named ERROR_ALREADY_EXISTS if it stays. It's confusing to readers if we invent our own names for well-known error codes.

Thanks for catching this detail about the appropriate name. Changed in commit 868b83d (at least until it is no longer needed).

These have now been (re-)moved to _winapi per your later suggestion as part of commit 6878533.

eryksun · 2019-02-20T08:02:47Z

Lib/multiprocessing/shared_memory.py

+            try:
+                last_error_code = _winapi.GetLastError()
+                if last_error_code == _winapi.ERROR_ALREADY_EXISTS:
+                    raise FileExistsError(f"File exists: {name!r}")


It would be nice to include the errno and winerror values for completeness: FileExistsError(errno.EEXIST, os.strerror(errno.EEXIST), name, _winapi.ERROR_ALREADY_EXISTS).

Good point -- added in commit 6878533.

eryksun · 2019-02-20T09:08:50Z

Lib/multiprocessing/shared_memory.py

+            # Attempt to dynamically determine the existing named shared
+            # memory block's size which is likely a multiple of mmap.PAGESIZE.
+            try:
+                h_map = _winapi.OpenFileMappingW(PAGE_READONLY, False, name)


This should request FILE_MAP_READ (4) access. PAGE_READONLY (2) is a page protection for CreateFileMapping. It happens to be the same constant value as FILE_MAP_WRITE access.

CreateFileMapping determines the desired access for the section object from the given page protection. OTOH, with OpenFileMapping we have to supply the desired access explicitly.

Thanks for catching this. Changed to _winapi.FILE_MAP_READ in commit 6878533.

eryksun · 2019-02-20T09:11:41Z

Lib/multiprocessing/shared_memory.py

+            except OSError:
+                raise FileNotFoundError(name)
+            try:
+                p_buf = _winapi.MapViewOfFile(h_map, PAGE_READONLY, 0, 0, 0)


This should request FILE_MAP_READ (4) access.

It's the other way around in the underlying NT API NtMapViewOfSection. It takes the desired page protection and determines the required section access from the protection. So MapViewOfFile[Ex] actually reverses this to get the page protection in order to call NtMapViewOfSection.

Also changed to _winapi.FILE_MAP_READ in commit 6878533.

eryksun · 2019-02-20T09:15:41Z

Lib/multiprocessing/shared_memory.py

+            h_map = _winapi.CreateFileMappingW(
+                INVALID_HANDLE_VALUE,
+                _winapi.NULL,
+                PAGE_EXECUTE_READWRITE,


Why do use PAGE_EXECUTE_READWRITE protection here? mmap defaults to PAGE_READWRITE.

I remember mentally debating this choice but not the details. Your point that mmap defaults to PAGE_READWRITE should have dictated what was used but I somehow missed it.

Good catch -- changed to _winapi.PAGE_READWRITE in commit 6878533.

eryksun · 2019-02-20T09:57:00Z

Modules/_winapi.c

+    Py_END_ALLOW_THREADS
+
+    if (handle == NULL) {
+        PyErr_SetFromWindowsErr(0);


Consider using PyErr_SetFromWindowsErrWithUnicodeFilename(0, name).

Done -- changed in commit 6878533.

eryksun · 2019-02-20T10:11:10Z

Modules/_winapi.c

@@ -464,6 +474,41 @@ _winapi_CreateFile_impl(PyObject *module, LPCTSTR file_name,
    return handle;
 }

+/*[clinic input]
+_winapi.CreateFileMappingW -> HANDLE


Personally, I'm not against using the real name with the "W" suffix. But thus far this module has used the macro names such as _winapi.CreateFileMapping.

Actually, we don't define UNICODE, and we have a few functions in _winapi that link to the ANSI version and use LPCTSTR -> LPCSTR (i.e. const char *). This includes _winapi.CreateFile. It should instead call CreateFileW and use an LPCWSTR name.

Thanks for making a point of this. I would prefer to stick with the established naming pattern unless there was a real reason otherwise. I have changed it to _winapi.CreateFileMapping as you suggest and likewise dropped the "W" on _winapi.OpenFileMapping in commit 6878533.

eryksun · 2019-02-20T11:27:02Z

Lib/multiprocessing/shared_memory.py

+
+    PAGE_READONLY = 0x02
+    PAGE_EXECUTE_READWRITE = 0x04
+    INVALID_HANDLE_VALUE = -1


I'd add INVALID_HANDLE_VALUE and the memory API constants to _winapi.

WINAPI_CONSTANT(F_HANDLE, INVALID_HANDLE_VALUE); WINAPI_CONSTANT(F_DWORD, FILE_MAP_READ); WINAPI_CONSTANT(F_DWORD, PAGE_READWRITE);

Maybe add the rest, too, because why not.

WINAPI_CONSTANT(F_DWORD, FILE_MAP_COPY); WINAPI_CONSTANT(F_DWORD, FILE_MAP_WRITE); WINAPI_CONSTANT(F_DWORD, FILE_MAP_EXECUTE); WINAPI_CONSTANT(F_DWORD, FILE_MAP_ALL_ACCESS); WINAPI_CONSTANT(F_DWORD, SEC_IMAGE); WINAPI_CONSTANT(F_DWORD, SEC_RESERVE); WINAPI_CONSTANT(F_DWORD, SEC_COMMIT); WINAPI_CONSTANT(F_DWORD, SEC_NOCACHE); WINAPI_CONSTANT(F_DWORD, SEC_WRITECOMBINE); WINAPI_CONSTANT(F_DWORD, SEC_LARGE_PAGES); WINAPI_CONSTANT(F_DWORD, PAGE_NOACCESS); WINAPI_CONSTANT(F_DWORD, PAGE_READONLY); WINAPI_CONSTANT(F_DWORD, PAGE_WRITECOPY); WINAPI_CONSTANT(F_DWORD, PAGE_EXECUTE); WINAPI_CONSTANT(F_DWORD, PAGE_EXECUTE_READ); WINAPI_CONSTANT(F_DWORD, PAGE_EXECUTE_READWRITE); WINAPI_CONSTANT(F_DWORD, PAGE_EXECUTE_WRITECOPY); WINAPI_CONSTANT(F_DWORD, PAGE_GUARD); WINAPI_CONSTANT(F_DWORD, PAGE_NOCACHE); WINAPI_CONSTANT(F_DWORD, PAGE_WRITECOMBINE); WINAPI_CONSTANT(F_DWORD, MEM_COMMIT); WINAPI_CONSTANT(F_DWORD, MEM_RESERVE); WINAPI_CONSTANT(F_DWORD, MEM_FREE); WINAPI_CONSTANT(F_DWORD, MEM_PRIVATE); WINAPI_CONSTANT(F_DWORD, MEM_MAPPED); WINAPI_CONSTANT(F_DWORD, MEM_IMAGE);

Sounds plenty good to me -- all have been added to _winapi in commit 6878533. Thanks!

…owsNamedSharedMemory into one.

…hing.

eryksun · 2019-02-21T05:06:14Z

Modules/_winapi.c

+    Py_END_ALLOW_THREADS
+
+    if (handle == NULL) {
+        PyErr_SetFromWindowsErr(0);


Use PyErr_SetFromWindowsErrWithUnicodeFilename(0, name).

Changed in commit caf0a5d.

I should have thought to change this when you suggested similar on CreateFileMapping. Thanks!

giampaolo

Much better overall. I added some inline comments.

giampaolo · 2019-02-22T10:14:55Z

Lib/multiprocessing/shared_memory.py

@@ -138,34 +130,99 @@ def unlink(self):
        pass


-class PosixSharedMemory(_PosixSharedMemory):
+# FreeBSD (and perhaps other BSDs) limit names to 14 characters.
+_SHM_SAFE_NAME_LENGTH = 14


FWIW this limit still exists and affects at least Linux, Solaris and AIX. I have code in psutil which takes this into account.

giampaolo · 2019-02-22T10:21:18Z

Modules/_winapi.c

+        handle = INVALID_HANDLE_VALUE;
+    }
+
+    return handle;


Shouldn't you return NULL instead of INVALID_HANDLE_VALUE in case of error? It appears this way you'll return an int instead of causing an exception.

HANDLE_return_converter checks for INVALID_HANDLE_VALUE with an error set. For NULL it returns None, which supports GetStdHandle since a standard-handle value may be NULL. Also, since an error must also be set, GetCurrentProcess works with the same converter even though the value it returns is equivalent to INVALID_HANDLE_VALUE.

It's a bit confusing if you're not aware of how it's implemented. It would probably be better to have a special handle type for functions that fail with INVALID_HANDLE_VALUE, since by far the majority of WINAPI functions that return a handle (directly or as an out parameter) either use NULL or FALSE to indicate an error.

@eryksun: Would it make more sense to handle the change you're suggesting by opening a separate issue and PR for it?

I'm hesitant to make such a "code maintenance" change impacting unrelated code as part of this PR which is an "enhancement".

@eryksun: I will offer to help supply the patch if you will review it.

It's a minor change, but @zooba should approve it in a new issue before going forward with a PR. I'd rename the current type to HANDLE_I and add a new HANDLE type that uses NULL for errors.

Another maintenance issue is removing the use of the TCHAR API and unintentional linking with ANSI functions such as CreateFileA, which I think would be fine to fix here if you want. The internal use of the affected functions hasn't had a problem thus far because they use ASCII strings, e.g. for pipe names, but it may eventually cause an issue.

Let's capture all of that in a new issue on b.p.o.

giampaolo · 2019-02-22T10:21:50Z

Modules/_winapi.c

+        handle = INVALID_HANDLE_VALUE;
+    }
+
+    return handle;


giampaolo · 2019-02-22T10:22:10Z

Modules/_winapi.c

+    Py_END_ALLOW_THREADS
+
+    if (size_of_buf == 0)
+        PyErr_SetFromWindowsErr(0);


giampaolo · 2019-02-22T10:29:40Z

Lib/multiprocessing/shared_memory.py

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


 encoding = "utf8"


This is only used by ShareableList. Why not make it a class attribute of that class instead? Also not sure if there are use-cases where one may want to change this (if not it's probably better to set it private).

Though it did not get included, the plan was to also supply a ShareableDict and encoding was to be used across both. Anticipating that a ShareableDict will still land in the future, let's keep it outside the class but I agree it makes sense to make this private.

Changed to private in commit 12c097d.

giampaolo · 2019-02-22T10:32:23Z

Lib/multiprocessing/shared_memory.py



-class PosixSharedMemory(_PosixSharedMemory):
+O_CREX = os.O_CREAT | os.O_EXCL


Consider making this private (_O_CREX)

Good point. Changed to private in commit 12c097d.

giampaolo · 2019-02-22T10:37:14Z

Modules/_multiprocessing/posixshmem.c

+
+    if (fd < 0) {
+        if (!async_err)
+            PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, path);


looks like you want to Py_DECREF(path) here?

Perhaps I am missing something... It looks like PyUnicode_AsUTF8 does not create a new reference so there is nothing to decrement?

Sorry, you are right.

Modules/_multiprocessing/posixshmem.c

bedevere-bot · 2019-02-24T04:08:18Z

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

Lib/multiprocessing/shared_memory.py

jakirkham · 2019-05-29T00:04:42Z

Lib/multiprocessing/managers.py

@@ -8,7 +8,8 @@
 # Licensed to PSF under a Contributor Agreement.
 #

-__all__ = [ 'BaseManager', 'SyncManager', 'BaseProxy', 'Token' ]
+__all__ = [ 'BaseManager', 'SyncManager', 'BaseProxy', 'Token',
+            'SharedMemoryManager' ]


According to this comment, sounds like adding SharedMemoryManager to __all__ should be handled to the try...except below.

PS: Nice work on this btw!

applio added 13 commits February 2, 2019 15:34

Added tests for shared_memory submodule.

720a8ea

Added tests for ShareableList.

29a7f80

Fix bug in allocationn size during creation of empty ShareableList il…

c56e29c

…luminated by existing test run on Linux.

Initial set of docs for shared_memory module.

c36de70

Added docs for ShareableList, added doctree entry for shared_memory s…

3c89c7c

…ubmodule, name refactoring for greater clarity.

Added examples to SharedMemoryManager docs, for ease of documentation…

5f4ba8f

… switched away from exclusively registered functions to some explicit methods on SharedMemoryManager.

Wording tweaks to docs.

f9aaa11

Fix test failures on Windows.

2377cfd

Added tests around SharedMemoryManager.

6bfa560

Documentation tweaks.

eaf7888

Fix inappropriate test on Windows.

e166ed9

Further documentation tweaks.

0f18511

Fix bare exception.

a097dbb

the-knights-who-say-ni added the CLA signed label Feb 11, 2019

bedevere-bot added the awaiting merge label Feb 11, 2019

eamanu reviewed Feb 11, 2019

View reviewed changes

Removed __copyright__.

7c65017

applio mentioned this pull request Feb 11, 2019

bpo-35813: Added shared_memory submodule of multiprocessing. #11664

Merged

applio added 3 commits February 11, 2019 13:43

Fixed typo in doc, removed comment.

da7731d

Merge remote-tracking branch 'upstream/master' into enh-tests-shmem

242a5e9

Updated SharedMemoryManager preliminary tests to reflect change of no…

7bdfbbb

…t supporting all registered functions on SyncManager.

applio added the skip news label Feb 11, 2019

Added Sphinx doctest run controls.

eec4bb1

giampaolo requested changes Feb 11, 2019

View reviewed changes

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

bedevere-bot added the awaiting changes label Feb 11, 2019

applio added 3 commits February 11, 2019 22:15

CloseHandle should be in a finally block in case MapViewOfFile fails.

1076567

Missed opportunity to use with statement.

0be0531

Switch to self.addCleanup to spare long try/finally blocks and save o…

1e5341e

…ne indentation, change to use decorator to skip test instead.

osvenskan reviewed Feb 19, 2019

View reviewed changes

eryksun reviewed Feb 19, 2019

View reviewed changes

applio added 2 commits February 19, 2019 14:59

Update access to kernel32 on Windows per feedback from eryksun.

868b83d

Moved kernel32 calls to _winapi.

9d83b06

eryksun reviewed Feb 20, 2019

View reviewed changes

applio added 5 commits February 20, 2019 10:48

Removed ShareableList.copy as redundant.

715ded9

Changes to _winapi use from eryksun feedback.

6878533

Adopt simpler SharedMemory API, collapsing PosixSharedMemory and Wind…

0d3d06f

…owsNamedSharedMemory into one.

Fix missing docstring on class, add test for ignoring size when attac…

05e26dd

…hing.

Moved SharedMemoryManager to managers module, tweak to fragile test.

7a3c7e5

eryksun reviewed Feb 21, 2019

View reviewed changes

Tweak to exception in OpenFileMapping suggested by eryksun.

caf0a5d

giampaolo requested changes Feb 22, 2019

View reviewed changes

Mark a few dangling bits as private as suggested by Giampaolo.

12c097d

applio merged commit e895de3 into python:master Feb 24, 2019

bedevere-bot removed the awaiting changes label Feb 24, 2019

jameshamm reviewed Feb 27, 2019

View reviewed changes

Lib/multiprocessing/shared_memory.py Show resolved Hide resolved

jakirkham reviewed May 29, 2019

View reviewed changes

applio mentioned this pull request May 11, 2023

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

Open



		The following example demonstrates a practical use of the :class:`SharedMemory`
		class with `NumPy arrays <https://www.numpy.org/>`_, accessing the

		copying of data.


		.. class:: SharedMemory(name, flags=None, mode=0o600, size=0, read_only=False)


		def test_joinable_queue(self):
		self.test_queue("JoinableQueue")



		class PosixSharedMemory(_PosixSharedMemory):
		O_CREX = os.O_CREAT \| os.O_EXCL

bpo-35813: Tests and docs for shared_memory #11816

bpo-35813: Tests and docs for shared_memory #11816

Conversation

applio commented Feb 11, 2019 • edited by bedevere-bot Loading

eamanu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo Feb 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 11, 2019

osvenskan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo left a comment

applio commented Feb 11, 2019 •

edited by bedevere-bot

Loading

giampaolo Feb 11, 2019 •

edited

Loading

giampaolo Feb 11, 2019 •

edited

Loading

giampaolo Feb 16, 2019 •

edited

Loading