bpo-31249: Fix ref cycle in ThreadPoolExecutor #3178
Conversation
Lib/concurrent/futures/thread.py
Outdated
| @@ -148,4 +150,5 @@ def shutdown(self, wait=True): | |||
| if wait: | |||
| for t in self._threads: | |||
| t.join() | |||
| self._threads.clear() | |||
There was a problem hiding this comment.
This change is not strictly needed to fix the initial bug:
https://bugs.python.org/issue31249#msg300634
But it should help to prevent leaking dangling threads if the executor object lives longer than expected.
There was a problem hiding this comment.
I have the feeling you're getting obsessed about "dangling threads".
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
At least, this part should be under the if wait clause IMO.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;-)
There was a problem hiding this comment.
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.
|
given I didn't respond earlier and I see no more |
|
Already backported in #3253 so I'm removing the label. |
* 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()
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