bpo-35378: Link the lifetime of the pool to the pool's iterators and results #10852
Conversation
pablogsal
added
type-bug
|
CC: @tzickel |
|
A. We should also backport to 2.7 as well (since currently the patch is also there). |
Yes, but I prefer to have the implementation fixed before opening the manual backport.
Because the old API allows passing a cache that is not the one in the pool ( |
|
I added C. why not self._pool = self._cache = None ? |
Unless I am missing something. the cache does not point to the pool/keep alive the pool. If that were true, this issue will not be happening. On the other hand I suppose this does not hurt, specially when the cache is populated with other jobs. |
|
|
||
| def __init__(self, cache, callback, error_callback): | ||
| def __init__(self, pool, callback, error_callback, cache=None): | ||
| self._pool = pool |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
More long explanation available here:
|
Let's discuss on https://bugs.python.org/issue34172 |
| class ApplyResult(object): | ||
|
|
||
| def __init__(self, cache, callback, error_callback): | ||
| def __init__(self, pool, callback, error_callback, cache=None): |
There was a problem hiding this comment.
Why the additional cache argument?
|
I am going to close this PR because we need to re-apply the origin patch to master and the new preferred solution is the weak reference. |
6 participants
Co-authored-by: tzickel [email protected]
This PR fixes a regression introduced in #8450 that manifests as a hanging process when using a
multiprocessing.pool.imapor similar iterator objects without keeping a reference to the pool itself.https://bugs.python.org/issue35378