bpo-31249: Fix ref cycle in ThreadPoolExecutor by vstinner · Pull Request #3178 · python/cpython (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
WorkItem.run() of concurrent.futures used by ThreadPoolExecutor now
breaks a reference cycle between an exception object and the WorkItem
object.
ThreadPoolExecutor.shutdown(wait=True) now also clears the set of
threads.
https://bugs.python.org/issue31249
| @@ -148,4 +150,5 @@ def shutdown(self, wait=True): |
|---|
| if wait: |
| for t in self._threads: |
| t.join() |
| self._threads.clear() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling you're getting obsessed about "dangling threads".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pitrou: I have the feeling you're getting obsessed about "dangling threads".
That's true. In my experience, sometimes, it shows a larger issue. For example, https://bugs.python.org/issue31249 pointed a reference cycle which wasn't seen before.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on race conditions since 3-6 months, and I saw so many non deterministic behaviours. I would like to make Python test suite more reliable, more reproductible.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least, this part should be under the if wait clause IMO.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there is no race condition here. Besides the Thread object doesn't seem to hold onto any important data, so it shouldn't be a problem if isn't collected immediately.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least, this part should be under the if wait clause IMO.
I hesitated to not clear threads on shutdown(wait=False), but I don't see how clearing _theads can introduce any issue.
Anyway, I made the change ;-)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there is no race condition here. Besides the Thread object doesn't seem to hold onto any important data, so it shouldn't be a problem if isn't collected immediately.
Hum, you know what? I reverted my changes on shutdown() to only break the reference cycle in _WorkItem ;-) Let's keep the change as small as possible.
concurrent.futures: WorkItem.run() used by ThreadPoolExecutor now breaks a reference cycle between an exception object and the WorkItem object. ThreadPoolExecutor.shutdown() now also clears its threads set.
@gpshead, @pitrou: What do you think of the added "self._threads.clear()" statement?
given I didn't respond earlier and I see no more self._threads.clear() in the accepted change, this change looks good to me. 👍
Already backported in #3253 so I'm removing the label.
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request
- bpo-31249: Fix ref cycle in ThreadPoolExecutor
concurrent.futures: WorkItem.run() used by ThreadPoolExecutor now breaks a reference cycle between an exception object and the WorkItem object. ThreadPoolExecutor.shutdown() now also clears its threads set.
shutdown() now only clears threads if wait is true.
Revert changes on shutdown()