Imap from ThreadPool behaves unexpectedly

[lev-veshnyakov]

BPO	28699
Nosy	@elias6, @applio, @zhangyangyu, @lev-veshnyakov
PRs	bpo-28699: fix a bug that exception at the very first of a iterable would cause pools behave abnormaly #693 [3.6] bpo-28699: fix a bug that exception at the very first of a iterable would cause pools behave abnormaly #882 [3.5] bpo-28699: fix a bug that exception at the very first of a iterable would cause pools behave abnormaly #884
Files	issue_28699_lacks_test_but_check_unsorted_py3x.patch: lacks proper test, preliminary patch issue28699_try.patch issue_28699_guards_iterable_still_lacks_formal_test_py3x.patch: lacks formal tests, preliminary patch replaces prior issue_28699_repro.py: crude form of tests

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/applio'
closed_at = <Date 2017-03-29.04:55:02.186>
created_at = <Date 2016-11-15.18:22:19.799>
labels = ['3.7', 'type-bug', 'library']
title = 'Imap from ThreadPool behaves unexpectedly'
updated_at = <Date 2017-03-29.04:55:02.179>
user = 'https://github.com/lev-veshnyakov'

bugs.python.org fields:

activity = <Date 2017-03-29.04:55:02.179>
actor = 'xiang.zhang'
assignee = 'davin'
closed = True
closed_date = <Date 2017-03-29.04:55:02.186>
closer = 'xiang.zhang'
components = ['Library (Lib)']
creation = <Date 2016-11-15.18:22:19.799>
creator = 'lev-veshnyakov'
dependencies = []
files = ['45516', '45522', '45645', '45646']
hgrepos = []
issue_num = 28699
keywords = ['patch']
message_count = 17.0
messages = ['280872', '280958', '280959', '281020', '281029', '281040', '281056', '281059', '281060', '281741', '281742', '281847', '282171', '289754', '290764', '290765', '290766']
nosy_count = 5.0
nosy_names = ['elias', 'davin', 'xiang.zhang', 'lev-veshnyakov', 'fiete']
pr_nums = ['693', '882', '884']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue28699'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[lev-veshnyakov]

Consider the following code:

from multiprocessing.pool import ThreadPool

pool = ThreadPool(10)

def gen():
    yield 1 + '1' # here is an error

print(list(pool.imap(str, gen()))) # prints []
print(list(pool.map(str, gen()))) # raises TypeError

The difference is, that the line with imap prints an empty list, while the line with map raises an exception, as expected.

Change the above snippet of code, adding additional yield statement:

from multiprocessing.pool import ThreadPool

pool = ThreadPool(10)

def gen():
    yield 1
    yield 1 + '1' # here is an error

print(list(pool.imap(str, gen()))) # raises TypeError
print(list(pool.map(str, gen()))) # also would raise TypeError

So now both map and imap will raise the exception, as expected. Therefore I suppose the behavior of imap showed in the first case is wrong.

[applio]

To tie in the example given by @elias in bpo-28625, this inconsistency in behavior is not limited to ThreadPool -- it appears with a process Pool as well:

from multiprocessing import Pool


def double(x):
    return 2 * x

def get_numbers():
    raise Exception("oops")
    yield 1
    yield 2

>>> list(Pool(processes=2).imap(double, get_numbers()))  # returns list
[]
>>> list(Pool(processes=2).map(double, get_numbers()))
Traceback (most recent call last):
  ...
Exception: oops

def get_numbers_differently():
    yield 1
    raise Exception("oops")
    yield 2

>>> list(Pool(processes=2).imap(double, get_numbers_differently()))  # now we see exception
Traceback (most recent call last):
  ...
Exception: oops

[applio]

This inconsistent behavior in imap on both Pool and ThreadPool is not what I would expect.

[applio]

Though it still lacks a proper test, I'm attaching a preliminary patch to address the problematic behavior in 3.5/3.6/default in the hopes that others might help test it.

[zhangyangyu]

Hi Davin, could it be fixed like this?

diff -r 05a728e1da15 Lib/multiprocessing/pool.py
--- a/Lib/multiprocessing/pool.py	Wed Nov 16 16:35:53 2016 -0800
+++ b/Lib/multiprocessing/pool.py	Thu Nov 17 16:35:38 2016 +0800
@@ -398,7 +398,7 @@
             except Exception as ex:
                 job, ind = task[:2] if task else (0, 0)
                 if job in cache:
-                    cache[job]._set(ind + 1, (False, ex))
+                    cache[job]._set(ind + 1 if task else 0, (False, ex))
                 if set_length:
                     util.debug('doing set_length()')
                     set_length(i+1)

It seems to me the bug is _handle_tasks doesn't treat the exception correctly if it's on the very first. Every time it _set(ind + 1) since if there is any exception the task is the previous task and + 1 is needed. But if the exception occurs at the very first, task is None and the + 1 is not needed.

I am not very sure but the reported cases work correctly now:

list(Pool(processes=2).imap(double, get_numbers())) # raises error now
list(pool.imap(str, gen())) # raises error now

[zhangyangyu]

What's more, this case seems non-reentrant. Since there is no task in this case, the job id is always 0 which is not true. This means after the first time, we can not set even the exception.

[zhangyangyu]

Here is a patch which is just a try. I don't quite like the implementation but I can not figure out a better solution. The examples in this one and bpo-28696 seems to work and no test fails currently.

[applio]

@xiang.zhang: Your patch looks to be introducing a number of changes to the structure of the data being passed around between threads and even monitored/indirectly shared across processes. It's looking increasingly high risk to me.

We already have logic for handling exceptions arising during jobs but the one situation overlooked in this logic is if the exception occurs "quickly in an unfortunate order", meaning the exception is encountered and reported before any of the other individual tasks can complete and respond with a result. This oversight of logic can be addressed a couple of ways:

1. Add a flag to our IMapIterator to indicate when any exception is encountered.
2. Modify the tuples / data structures being maintained across IMapIterator's _cache, _items, _unsorted, _index, and _length.
3. Under relevant conditions, check both _items and _unsorted (not only _items) before declaring that we truly have all results in.

I regard option 1 as being potentially a bit fragile and option 2 as introducing non-trivial complexity and risk. With option 3, there's effectively no risk and no measurable cost getting to the truth of what has actually happened.

[zhangyangyu]

Your patch looks to be introducing a number of changes to the structure of the data being passed around between threads and even monitored/indirectly shared across processes.

That's why I say even myself don't like it. To solve an edge case some long introduced codes have to been changed. Sigh.

Under relevant conditions, check both _items and _unsorted (not only _items) before declaring that we truly have all results in.

I think you mean your patch. But how does it solve the reentrant problem? Yeah, actually it's not reentrant. If the problematic job is not the first job with id 0, then the exception won't be set.

With your patch, repeatedly execute print(list(pool.imap(str, gen()))). Only the first time there is an exception.

[applio]

@xiang.zhang: Nice catch -- thank you for pointing out the additional issue that arises when trying to provoke the issue twice in a row.

The module attribute job_counter helped, in this situation, point out what might have otherwise been overlooked.

I didn't like any of my previously suggested approaches, but I think there's a 4th option:
4. Guard against misbehaving generators/iterables *before* they are put into the taskqueue.

Now when we provide a problematic generator/iterable to imap, the exception it triggers is caught and the resulting exception passed through the system to make use of the logic that is already in place.

This same issue can arise for imap_unordered() as well as imap() and can be addressed in the same manner.

Attaching another preliminary patch that still lacks formal tests but I'll attach crude versions of tests momentarily.

If we're still missing some use case or other logic path, now's the time to find it.

[applio]

Attaching promised crude tests.