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
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
