Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(40)

#6766: Cannot modify dictionaries inside dictionaries using Managers from multiprocessing

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 2 months ago by carlosdf
Modified:
1 year, 3 months ago
Reviewers:
yselivanov, python
CC:
jnoller_gmail.com, carlosdf_gmail.com, terrence_zettabytestorage.com, kghose_users.sf.net, devnull_psf.upfronthosting.co.za, sbt, dario.suarez_telefonica.net, Yury Selivanov, fothergill_gmail.com, dan.oreilly, waldemar.parzonka_gmail.com, davin, papercrane_reversefold.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 10

Patch Set 3 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/multiprocessing.rst View 1 2 6 chunks +55 lines, -30 lines 0 comments Download
Lib/multiprocessing/managers.py View 1 2 10 chunks +70 lines, -25 lines 0 comments Download
Lib/test/_test_multiprocessing.py View 1 2 2 chunks +68 lines, -2 lines 0 comments Download

Messages

Total messages: 2
Yury Selivanov
http://bugs.python.org/review/6766/diff/18401/Doc/library/multiprocessing.rst File Doc/library/multiprocessing.rst (right): http://bugs.python.org/review/6766/diff/18401/Doc/library/multiprocessing.rst#newcode1755 Doc/library/multiprocessing.rst:1755: Create a shared :class:`list` object and return a proxy ...
1 year, 3 months ago #1
davin
1 year, 3 months ago #2
Agreed on all points.  Updated patch forthcoming.

http://bugs.python.org/review/6766/diff/18401/Doc/library/multiprocessing.rst
File Doc/library/multiprocessing.rst (right):

http://bugs.python.org/review/6766/diff/18401/Doc/library/multiprocessing.rst...
Doc/library/multiprocessing.rst:1755: Create a shared :class:`list` object and
return a proxy for it.
Good point.  Added at the bottom of the SyncManager class section as the change
applies to all of the shared objects it offers.

On 2016/09/07 23:36:50, Yury Selivanov wrote:
> Need to add "versionchanged" block with an explanation about your changes in
> 3.6.

http://bugs.python.org/review/6766/diff/18401/Doc/library/multiprocessing.rst...
Doc/library/multiprocessing.rst:1901: :ref:`multiprocessing-proxy_objects`. 
This permits a nesting of these managed
Total overkill:  I ran it past a technical editor who suggested that it isn't
wrong but without the "a" would be better.

Sounds good to me -- I've removed the "a".

On 2016/09/07 23:36:50, Yury Selivanov wrote:
> "permits a nesting" -- do we need the "a" article here?

http://bugs.python.org/review/6766/diff/18401/Lib/multiprocessing/managers.py
File Lib/multiprocessing/managers.py (right):

http://bugs.python.org/review/6766/diff/18401/Lib/multiprocessing/managers.py...
Lib/multiprocessing/managers.py:426: self.id_to_obj[ident] = (None, (), None)  #
thread-safe
Perhaps this expanded comment might help:
    # Two-step process in case the object turns out to contain other
    # proxy objects (e.g. a managed list of managed lists).
    # Otherwise, deleting self.id_to_obj[ident] would trigger the
    # deleting of the stored value (another managed object) which would
    # in turn attempt to acquire the mutex that is already held here.


On 2016/09/07 23:36:50, Yury Selivanov wrote:
> Is it possible to have a little bit more detailed comment here explaining why
we
> add/remove the same key?

http://bugs.python.org/review/6766/diff/18401/Lib/test/_test_multiprocessing.py
File Lib/test/_test_multiprocessing.py (right):

http://bugs.python.org/review/6766/diff/18401/Lib/test/_test_multiprocessing....
Lib/test/_test_multiprocessing.py:1708: self.assertEqual(outer[-1][-1]['feed'],
3)
Good idea.  We don't expose clear on list (don't ask me why) but we do on dict. 
This should be handled well and while other modification / deletion operations
that tickle some of the same code paths used by clear are being tested on nested
managed objects, it's good to add a test on calling dict.clear as well.  I've
now added a quick test of dict.clear to this same test method.

On 2016/09/07 23:36:50, Yury Selivanov wrote:
> Do we need to test `list.clear()`/`dict.clear()` and other methods (when
> nested)? Or they are already covered?

http://bugs.python.org/review/6766/diff/18401/Lib/test/_test_multiprocessing....
Lib/test/_test_multiprocessing.py:3920: t = 0.01
Good catch.  Yes, it was in fact left over from a temporary addition to this
test.  There's no reason for it to be kept -- I'm taking it out.

On 2016/09/07 23:36:50, Yury Selivanov wrote:
> Is this some debug code?
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7