bpo-39995: CLN remove some locks in ProcessPoolExecutor #19788
Conversation
| @@ -658,7 +660,8 @@ def _start_executor_manager_thread(self): | |||
| self._executor_manager_thread = _ExecutorManagerThread(self) | |||
| self._executor_manager_thread.start() | |||
| _threads_wakeups[self._executor_manager_thread] = \ | |||
| self._executor_manager_thread_wakeup | |||
| (self._shutdown_lock, | |||
| self._executor_manager_thread_wakeup) | |||
There was a problem hiding this comment.
Oh, I never tried a weakref.WeakKeyDictionary() where the value a tuple of two objects.
There was a problem hiding this comment.
Do you think this could be a problem? I did not even thing about this..
There was a problem hiding this comment.
As long as they key isn't a temporary object this is fine.
|
@tomMoral @pitrou: What's the status of this PR? Is it worth it? I would like to close https://bugs.python.org/issue39995 since the initial issue has been fixed :-) |
|
I would this PR improves a bit the state with the protection of the at exit call |
|
https://bugs.python.org/issue39995 is closed. What is the status of this PR? |
|
This PR is stale because it has been open for 30 days with no activity. |
Follow-up of #19760 to try to remove some locks.
Actually, the only one that matters in term of perf is probably the one in Lib/concurrent/futures/process.py#L171 which would be concurrent with the job submission. It can be removed because this lock is here to protect against concurrent call to
closewhich is only called in the same thread asclearso the protection is unecesary.We can also remove the one in
SafeQueuebecause the queue is closed (and thus the queue feeder thread join) before we callthread_wakeup.close. But this is not really changing the perf so maybe we could leave it. Let me know what you prefer.Finally, the call to
wakeupin_python_exitwas indeed not protected and this could lead to the same race condition. I added a lock here to make sure we avoid this. (A bit more complicated than expected as the_python_exitwas leaking in the workers).I launched 50times the tests with @vstinner patch and did not observed the failure so I think this is working.
https://bugs.python.org/issue39995