Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
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
|
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. |
| @@ -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): | |||
| self._pool = pool | |||
There was a problem 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.
There was a problem 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.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
More long explanation available here:
|
Let's discuss on https://bugs.python.org/issue34172 |
| @@ -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): | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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. |
Co-authored-by: tzickel tzickel@users.noreply.github.com
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