bpo-39995: Fix concurrent.futures _ThreadWakeup#19760
bpo-39995: Fix concurrent.futures _ThreadWakeup#19760vstinner merged 1 commit intopython:masterfrom vstinner:futures_shutdown_lock
Conversation
Fix a race condition in concurrent.futures._ThreadWakeup: access to _ThreadWakeup is now protected with the shutdown lock.
| with self._shutdown_lock: | ||
| self._cancel_pending_futures = cancel_futures | ||
| self._shutdown_thread = True | ||
| if self._executor_manager_thread_wakeup is not None: |
There was a problem hiding this comment.
I think this is the same as both are set to None in the same call to shutdown. (L.744/750)
This is more explicit this way so perfect.
|
This PR is based on @tomMoral (Thomas Moreau) idea:
I wrote a conservative change which always lock ProcessPoolExecutor._shutdown_lock while accessing _ThreadWakeup because I don't know the concurrent.futures module. Maybe we can avoid the lock in some cases. |
|
@pitrou @tomMoral: https://bugs.python.org/issue39995 is becoming more and more annoying for the Python CI. I suggest to start with a conservative approach to unblock https://bugs.python.org/issue39995 and then investigate where we can avoid locking. Right now, tons of test_concurrent_futures tests are failing randomly and it's becoming hard to handle all these failures and identify root issues. |
|
I tested manually: this PR fix test_killed_child() and ProcessPoolForkExecutorDeadlockTest tests. Using my https://bugs.python.org/msg367463 patch (sleep), without this change: it is very easy to reproduce the bug. With this change, tests no longer fail. In short, with https://bugs.python.org/msg367463 patch (sleep) and this change, only test_cancel_futures() fails: but that's expected, see https://bugs.python.org/issue39995#msg367544 |
| is_broken = False | ||
| self.thread_wakeup.clear() | ||
|
|
||
| with self.shutdown_lock: |
There was a problem hiding this comment.
I don't think this lock is necessary as the goal is to protect against _ThreadWakeup.close which should only be called in the same thread.
4 participants
Fix a race condition in concurrent.futures._ThreadWakeup: access to
_ThreadWakeup is now protected with the shutdown lock.
https://bugs.python.org/issue39995