[Python-Dev] Asking for reversion

[Giampaolo Rodola']

Davin,
I am not familiar with the multiprocessing module, so take the following
with a big grain of salt. I took a look at the PR, then I got an idea of
how multiprocessing module is organized by reading the doc. Here's some
food for thought in terms of API reorganization.

SharedMemoryManager, SharedMemoryServer
---------------------------------------

It appears to me these are the 2 main public classes, and after reading the
doc it seems they really belong to "managers
<https://docs.python.org/3/library/multiprocessing.html#multiprocessing-managers>"
(multiprocessing.managers namespace). Also:
* SharedMemoryManager is a subclass of multiprocessing.managers.SyncManager
* SharedMemoryServer is a subclass of multiprocessing.managers.Server
shared_memory.py could be renamed to _shared_memory.py and managers.py
could import and expose these 2 classes only.

Support APIs
------------

These are objects which seem to be used in support of the 2 classes above,
but apparently are not meant to be public. As such they could simply live
in _shared_memory.py and not be exposed:

- shareable_wrap(): used only in SharedMemoryTracker.wrap()
- SharedMemoryTracker: used only by SharedMemoryServer
- SharedMemory, WindowsNamedSharedMemory, PosixSharedMemory: used by
shareable_wrap() and SharedMemoryTracker
- ShareableList: it appears this is not used, but by reading here
<https://github.com/python/cpython/blob/e0e5065daef36dafe10a46eaa8b7800274d73062/Lib/multiprocessing/managers.py#L1194>
I have a doubt: shouldn't it be register()ed against SharedMemoryManager?

C extension module
------------------

- ExistentialError, Error - it appears these are not used
- PermissionsException, ExistentialException - I concur with Ronald
Oussoren's review: you could simply use PyErr_SetFromErrno() and let the
original OSError exception bubble up. Same for O_CREAT, O_EXCL, O_CREX,
O_TRUNC which are already exposed in the os module. I have a couple of
other minor nitpicks re. the code but I will comment on the PR.

Compatibility
-------------

I'm not sure if SyncManager and SharedMemoryManager are fully
interchangeable so I think the doc should clarify this. SyncManager handles
a certain set of types
<https://github.com/python/cpython/blob/e0e5065daef36dafe10a46eaa8b7800274d73062/Lib/multiprocessing/managers.py#L1183>.
It appears SharedMemoryManager is supposedly able to do the same except for
lists
<https://github.com/applio/cpython/blob/516cf4ac14af257913f46216973c09d58eb259d5/Lib/multiprocessing/shared_memory.py#L227>.
Is my assumption correct? Also, multiprocessing.Manager() by default
returns a SyncManager. If we'll get to a point where SyncManager and
SharedMemoryManager are able to handle the same types it'd be good to
return SharedMemoryManager as the default, but it's probably safer to leave
it for later. Unless they are already there (I don't know) it would be good
to have a full set of unit-tests for all the register()ed types and test
them against SyncManager and SharedMemoryManager. That would give an idea
on the real interchangeability of these 2 classes and would also help
writing a comprehensive doc.

Hope this helps.

-- 
Giampaolo - http://grodola.blogspot.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-dev/attachments/20190206/aa2e949f/attachment.html>