pythongh-109917, pythongh-105829: Fix concurrent.futures _ThreadWakeu…

…p.wakeup() Remove test_gh105829_should_not_deadlock_if_wakeup_pipe_full() of test_concurrent_futures.test_deadlock. The test is no longer relevant.
vstinner · Sep 29, 2023 · 531aca8 · 531aca8
1 parent 5ae6c6d
commit 531aca8
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 82 deletions.
diff --git a/Lib/concurrent/futures/process.py b/Lib/concurrent/futures/process.py
@@ -69,26 +69,37 @@ class _ThreadWakeup:
     def __init__(self):
         self._closed = False
         self._reader, self._writer = mp.Pipe(duplex=False)
+        self._awaken = False
 
     def close(self):
         # Please note that we do not take the shutdown lock when
         # calling clear() (to avoid deadlocking) so this method can
         # only be called safely from the same thread as all calls to
         # clear() even if you hold the shutdown lock. Otherwise we
         # might try to read from the closed pipe.
-        if not self._closed:
-            self._closed = True
-            self._writer.close()
-            self._reader.close()
+        if self._closed:
+            return
+        self._closed = True
+        self._writer.close()
+        self._reader.close()
 
     def wakeup(self):
-        if not self._closed:
-            self._writer.send_bytes(b"")
+        if self._closed:
+            return
+        if self._awaken:
+            # gh-105829: Write a single byte to not block if the pipe is full.
+            # wait_result_broken_or_wakeup() does not even read the written
+            # byte. It only check if something was written to the pipe.
+            return
+        self._writer.send_bytes(b"x")
+        #self._awaken = True
 
     def clear(self):
-        if not self._closed:
-            while self._reader.poll():
-                self._reader.recv_bytes()
+        if self._closed:
+            return
+        while self._reader.poll():
+            self._reader.recv_bytes()
+        self._awaken = False
 
 
 def _python_exit():
@@ -438,12 +449,8 @@ def wait_result_broken_or_wakeup(self):
         elif wakeup_reader in ready:
             is_broken = False
 
-        # No need to hold the _shutdown_lock here because:
-        # 1. we're the only thread to use the wakeup reader
-        # 2. we're also the only thread to call thread_wakeup.close()
-        # 3. we want to avoid a possible deadlock when both reader and writer
-        #    would block (gh-105829)
-        self.thread_wakeup.clear()
+        with self.shutdown_lock:
+            self.thread_wakeup.clear()
 
         return result_item, is_broken, cause
 

diff --git a/Lib/test/test_concurrent_futures/test_deadlock.py b/Lib/test/test_concurrent_futures/test_deadlock.py
@@ -250,73 +250,6 @@ def test_crash_big_data(self):
 
         executor.shutdown(wait=True)
 
-    def test_gh105829_should_not_deadlock_if_wakeup_pipe_full(self):
-        # Issue #105829: The _ExecutorManagerThread wakeup pipe could
-        # fill up and block. See: https://github.com/python/cpython/issues/105829
-
-        # Lots of cargo culting while writing this test, apologies if
-        # something is really stupid...
-
-        self.executor.shutdown(wait=True)
-
-        if not hasattr(signal, 'alarm'):
-            raise unittest.SkipTest(
-                "Tested platform does not support the alarm signal")
-
-        def timeout(_signum, _frame):
-            import faulthandler
-            faulthandler.dump_traceback()
-
-            raise RuntimeError("timed out while submitting jobs?")
-
-        thread_run = futures.process._ExecutorManagerThread.run
-        def mock_run(self):
-            # Delay thread startup so the wakeup pipe can fill up and block
-            time.sleep(3)
-            thread_run(self)
-
-        class MockWakeup(_ThreadWakeup):
-            """Mock wakeup object to force the wakeup to block"""
-            def __init__(self):
-                super().__init__()
-                self._dummy_queue = queue.Queue(maxsize=1)
-
-            def wakeup(self):
-                self._dummy_queue.put(None, block=True)
-                super().wakeup()
-
-            def clear(self):
-                try:
-                    while True:
-                        self._dummy_queue.get_nowait()
-                except queue.Empty:
-                    super().clear()
-
-        with (unittest.mock.patch.object(futures.process._ExecutorManagerThread,
-                                         'run', mock_run),
-              unittest.mock.patch('concurrent.futures.process._ThreadWakeup',
-                                  MockWakeup)):
-            with self.executor_type(max_workers=2,
-                                    mp_context=self.get_context()) as executor:
-                self.executor = executor  # Allow clean up in fail_on_deadlock
-
-                job_num = 100
-                job_data = range(job_num)
-
-                # Need to use sigalarm for timeout detection because
-                # Executor.submit is not guarded by any timeout (both
-                # self._work_ids.put(self._queue_count) and
-                # self._executor_manager_thread_wakeup.wakeup() might
-                # timeout, maybe more?). In this specific case it was
-                # the wakeup call that deadlocked on a blocking pipe.
-                old_handler = signal.signal(signal.SIGALRM, timeout)
-                try:
-                    signal.alarm(int(self.TIMEOUT))
-                    self.assertEqual(job_num, len(list(executor.map(int, job_data))))
-                finally:
-                    signal.alarm(0)
-                    signal.signal(signal.SIGALRM, old_handler)
-
 
 create_executor_tests(globals(), ExecutorDeadlockTest,
                       executor_mixins=(ProcessPoolForkMixin,