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

#23051: multiprocessing.pool methods imap() and imap_unordered() cause deadlock

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by diamant.alon
Modified:
2 years, 6 months ago
Reviewers:
storchaka, python
CC:
AntoinePitrou, devnull_psf.upfronthosting.co.za, sbt, storchaka, josh.rosenberg, davin, diamant.alon_gmail.com, gregory.szorc_gmail.com
Visibility:
Public.

Patch Set 1 #

Total comments: 9

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/multiprocessing/pool.py View 1 2 3 4 2 chunks +20 lines, -8 lines 0 comments Download
Lib/test/_test_multiprocessing.py View 1 2 3 4 3 chunks +46 lines, -0 lines 0 comments Download

Messages

Total messages: 4
storchaka_gmail.com
https://bugs.python.org/review/23051/diff/14005/Lib/multiprocessing/pool.py File Lib/multiprocessing/pool.py (right): https://bugs.python.org/review/23051/diff/14005/Lib/multiprocessing/pool.py#newcode377 Lib/multiprocessing/pool.py:377: task = None May be set task = (0, ...
2 years, 6 months ago #1
davin
Apparently these are not appearing inline here in Rietveld. I will try to respond to ...
2 years, 6 months ago #2
storchaka_gmail.com
> Apparently these are not appearing inline here in Rietveld. I will try to > ...
2 years, 6 months ago #3
davin
2 years, 6 months ago #4
Hi Serhiy --

I can not reproduce it, but after refreshing the review page on Rietveld, now I
can see your comments inline and respond to them in the usual way.  For whatever
reason, it was showing 0 comments on all the files in all the patch sets yet it
would show me the contents of the emailed message down below.  I am pretty sure
I refreshed the page several times before when trying to figure out why on earth
it wasn't letting me see the comments inline.  Whatever happened, it's working
as expected again.

Thanks again for taking the time to walk through these things with me.


Davin

https://bugs.python.org/review/23051/diff/14005/Lib/test/_test_multiprocessin...
File Lib/test/_test_multiprocessing.py (right):

https://bugs.python.org/review/23051/diff/14005/Lib/test/_test_multiprocessin...
Lib/test/_test_multiprocessing.py:1753: self.assertRaises(ValueError,
it.__next__)
On 2015/03/06 20:28:15, storchaka wrote:
> > > Why not self.assertRaises(ValueError, next, it)?
> > 
> > That would be fine too.  In Python3, multiprocessing's IMapIterator now has
> both
> > a __next__ and a next method defined so consideration was given to the
> potential
> > for confusion between the builtin next versus IMapIterator.next -- given
that
> > small potential for confusion and to contrast with the Python2 version of
this
> > test, the idea was to make explicit that we are leveraging __next__ in this
> test
> > whereas in the corresponding Python2 patch, we can not and do not.  If the
> > notion of calling a dunder-method directly as part of a unit test for that
> > module is not a best practice, I'll gladly change it -- please tell me what
> the
> > recommended practice is to do here.
> 
> I meant not IMapIterator.next, but builtin next(). One line above builtin
next()
> is used to obtain a value.
> 
> I suppose this is a legacy from Python 2 where it.next() was official way to
> obtain a value from the iterator.

Yes, definitely a legacy holdover.  Actually, I understood that you meant
builtin next and not IMapIterator.next -- my advocation for using
IMapIterator.__next__ here was to highlight that we are indeed exercising
IMapIterator.__next__ (or __builtins__.next) and not IMapIterator.next.  For
someone looking at the Python2 and Python3 versions of these tests, I thought it
a valuable thing to highlight.


> > Why assertRaisesRegexp?  Or did you just suggest that as some sort of other
> > distinctive exception type?  In the implementation for
> > exception_throwing_generator, I don't see another opportunity for a
ValueError
> > to be raised -- am I missing one though?  If it truly is the only way the
> > ValueError can be raised, and given ValueError is a relevant/appropriate
> choice,
> > wouldn't a custom exception be overkill here?  Happy to do it here and on
line
> > 1779 if you think it will improve things but I'm wanting to understand.
> 
> I meant that ValueError can be not an exception raised by
> exception_throwing_generator() but an exception raised by imap implementation
> due to unknown bug (current or future). self.assertRaises(ValueError) will
hide
> this bug. But only a ValueError raised by exception_throwing_generator() has
> error message "Somebody said when", and when use custom exception, only
> exception raised by exception_throwing_generator() will match this exception
> type.


Now I understand better -- thanks.  Future-proofing is a great point.  I will
add a custom exception class as you suggest.  This will be nicer/cleaner than
needing to test the exception message to see if it also matches.
Sign in to reply to this message.

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