Skip to content

bpo-35378: Link the lifetime of the pool to the pool's iterators and results #10852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pablogsal wants to merge 2 commits into from
Closed

bpo-35378: Link the lifetime of the pool to the pool's iterators and results #10852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b4d921
bpo-35378: Link the lifetime of the pool to the pool's iterators and …
pablogsal Dec 2, 2018
171dab0
Use a longer (parametrised) timeout for the slower buildbots
pablogsal Dec 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions Lib/multiprocessing/pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ def imap(self, func, iterable, chunksize=1):
if self._state != RUN:
raise ValueError("Pool not running")
if chunksize == 1:
result = IMapIterator(self._cache)
result = IMapIterator(self)
self._taskqueue.put(
(
self._guarded_task_generation(result._job, func, iterable),
Expand All @@ -344,7 +344,7 @@ def imap(self, func, iterable, chunksize=1):
"Chunksize must be 1+, not {0:n}".format(
chunksize))
task_batches = Pool._get_tasks(func, iterable, chunksize)
result = IMapIterator(self._cache)
result = IMapIterator(self)
self._taskqueue.put(
(
self._guarded_task_generation(result._job,
Expand All @@ -361,7 +361,7 @@ def imap_unordered(self, func, iterable, chunksize=1):
if self._state != RUN:
raise ValueError("Pool not running")
if chunksize == 1:
result = IMapUnorderedIterator(self._cache)
result = IMapUnorderedIterator(self)
self._taskqueue.put(
(
self._guarded_task_generation(result._job, func, iterable),
Expand All @@ -373,7 +373,7 @@ def imap_unordered(self, func, iterable, chunksize=1):
raise ValueError(
"Chunksize must be 1+, not {0!r}".format(chunksize))
task_batches = Pool._get_tasks(func, iterable, chunksize)
result = IMapUnorderedIterator(self._cache)
result = IMapUnorderedIterator(self)
self._taskqueue.put(
(
self._guarded_task_generation(result._job,
Expand All @@ -390,7 +390,7 @@ def apply_async(self, func, args=(), kwds={}, callback=None,
'''
if self._state != RUN:
raise ValueError("Pool not running")
result = ApplyResult(self._cache, callback, error_callback)
result = ApplyResult(self, callback, error_callback)
self._taskqueue.put(([(result._job, 0, func, args, kwds)], None))
return result

Expand Down Expand Up @@ -420,7 +420,7 @@ def _map_async(self, func, iterable, mapper, chunksize=None, callback=None,
chunksize = 0

task_batches = Pool._get_tasks(func, iterable, chunksize)
result = MapResult(self._cache, chunksize, len(iterable), callback,
result = MapResult(self, chunksize, len(iterable), callback,
error_callback=error_callback)
self._taskqueue.put(
(
Expand Down Expand Up @@ -662,13 +662,14 @@ def __exit__(self, exc_type, exc_val, exc_tb):

class ApplyResult(object):

def __init__(self, cache, callback, error_callback):
def __init__(self, pool, callback, error_callback, cache=None):

@pitrou pitrou Dec 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the additional cache argument?

self._pool = pool

@vstinner vstinner Dec 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote "This PR fixes a regression introduced in PR #8450" which comes from https://bugs.python.org/issue34172. This issue is a reference cycle fix if I understand correctly, but here you create a new strong reference to the pool. I see a risk of creating new reference cycles. No?

Before https://bugs.python.org/issue34172 fix, ApplyResult didn't hold a reference to the pool: you add a new reference.

Maybe it's fine, but I'm just concerned that this PR might reintroduce a bug similar but different than https://bugs.python.org/issue34172

I don't understand why you need to hold a strong reference to the pool.

@pablogsal pablogsal Dec 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to hold a reference to the pool because the regression is that the pool may be destroyed while one of its iterators is still alive if no references are left to the pool. In this case, iterating over the iterator of the pool hangs. The solution links the lifetime of the pool to the lifetime of the iterator (basically: keeping the pool alive while the iterator is alive). When the iterator is exhausted, the references to the pool are cleaned.

The issue in #8450 is different, is a reference cycle between the Pool object, and the self._worker_handler Thread object, which is more fundamental.

In this case, the cycle needs to exist while the iterator is alive: all objects that depend on the pool need to make sure that the pool is alive while they are still in use (a non-consumed iterator...etc), otherwise hangs or inconsistent state can happen. This did not happen before #8450 because of the reference cycle between the Pool object and self._worker_handler prevented the pool to be destroyed.

This PR makes sure to break the cycle when the pool objects are not needed anymore.

@pablogsal pablogsal Dec 3, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More long explanation available here:

https://bugs.python.org/msg330996

self._event = threading.Event()
self._job = next(job_counter)
self._cache = cache
self._cache = cache if cache is not None else pool._cache
self._callback = callback
self._error_callback = error_callback
cache[self._job] = self
self._cache[self._job] = self

def ready(self):
return self._event.is_set()
Expand Down Expand Up @@ -698,6 +699,7 @@ def _set(self, i, obj):
self._error_callback(self._value)
self._event.set()
del self._cache[self._job]
self._pool = self._cache = None

AsyncResult = ApplyResult # create alias -- see #17805

Expand All @@ -707,16 +709,17 @@ def _set(self, i, obj):

class MapResult(ApplyResult):

def __init__(self, cache, chunksize, length, callback, error_callback):
ApplyResult.__init__(self, cache, callback,
def __init__(self, pool, chunksize, length, callback, error_callback, cache=None):
ApplyResult.__init__(self, pool, callback,
error_callback=error_callback)
self._success = True
self._value = [None] * length
self._chunksize = chunksize
if chunksize <= 0:
self._number_left = 0
self._event.set()
del cache[self._job]
del self._cache[self._job]
Comment thread
pablogsal marked this conversation as resolved.
Outdated
self._pool = self._cache = None
else:
self._number_left = length//chunksize + bool(length % chunksize)

Expand All @@ -730,6 +733,7 @@ def _set(self, i, success_result):
self._callback(self._value)
del self._cache[self._job]
self._event.set()
self._pool = self._cache = None
else:
if not success and self._success:
# only store first exception
Expand All @@ -741,22 +745,24 @@ def _set(self, i, success_result):
self._error_callback(self._value)
del self._cache[self._job]
self._event.set()
self._pool = self._cache = None

#
# Class whose instances are returned by `Pool.imap()`
#

class IMapIterator(object):

def __init__(self, cache):
def __init__(self, pool, cache=None):
self._pool = pool
self._cond = threading.Condition(threading.Lock())
self._job = next(job_counter)
self._cache = cache
self._cache = cache if cache is not None else pool._cache
self._items = collections.deque()
self._index = 0
self._length = None
self._unsorted = {}
cache[self._job] = self
self._cache[self._job] = self

def __iter__(self):
return self
Expand All @@ -767,6 +773,7 @@ def next(self, timeout=None):
item = self._items.popleft()
except IndexError:
if self._index == self._length:
self._pool = self._cache = None
raise StopIteration from None
self._cond.wait(timeout)
try:
Expand Down Expand Up @@ -798,13 +805,15 @@ def _set(self, i, obj):

if self._index == self._length:
del self._cache[self._job]
self._pool = self._cache = None

def _set_length(self, length):
with self._cond:
self._length = length
if self._index == self._length:
self._cond.notify()
del self._cache[self._job]
self._pool = self._cache = None

#
# Class whose instances are returned by `Pool.imap_unordered()`
Expand All @@ -819,6 +828,7 @@ def _set(self, i, obj):
self._cond.notify()
if self._index == self._length:
del self._cache[self._job]
self._pool = self._cache = None

#
#
Expand Down
35 changes: 35 additions & 0 deletions Lib/test/_test_multiprocessing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2566,6 +2566,41 @@ def test_del_pool(self):
gc.collect()
self.assertIsNone(wr())

@support.reap_threads
def test_pool_iterators_and_results_keeps_alive_the_pool_and_does_not_hang(self):
# Check bpo35378
def _test_func(func, args, test_body):
result = getattr(self.Pool(), func)(*args)
# If the pool is used as a context manager, *result* is a proxy object,
# but then we won't need to close and join as the context manager will do
# it for us.
if hasattr(result, "_pool"):
close_func = result._pool.close
join_func = result._pool.join
else:
close_func = lambda: None
join_func = lambda: None

try:
test_body(result)
finally:
close_func()
join_func()
del result

test_functions = [
("imap", (str, range(100)),lambda result: list(result)),
("imap_unordered", (str, range(100)),lambda result: list(result)),
("apply_async", (str, [10]),lambda result: result.get()),
("map_async", (str, range(100)),lambda result: result.get()),
]

for args in test_functions:
p = threading.Thread(target=_test_func, args=args)
p.start()
p.join(timeout=TIMEOUT)
self.assertFalse(p.is_alive())

def raising():
raise KeyError("key")

Expand Down