From 98ac8981de7b1818f0d1d7042878399a7d966754 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 5 Oct 2023 19:30:31 +0200 Subject: [PATCH] gh-110167: Fix test_socket deadlock in doCleanups() Fix a deadlock in test_socket when server fails with a timeout but the client is still running in its thread. Don't hold a lock to call cleanup functions in doCleanups(). One of the cleanup function waits until the client completes, whereas the client could deadlock if it called addCleanup() in such situation. doCleanups() is called when the server completed, but the client can still be running in its thread especially if the server failed with a timeout. Don't put a lock on doCleanups() to prevent deadlock between addCleanup() called in the client and doCleanups() waiting for self.done.wait of ThreadableTest._setUp(). --- Lib/test/test_socket.py | 12 +++++++----- .../2023-10-05-19-33-49.gh-issue-110167.mIdj3v.rst | 5 +++++ 2 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/Tests/2023-10-05-19-33-49.gh-issue-110167.mIdj3v.rst diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 09605f7e774dda..0d1c1867a25c40 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -218,8 +218,13 @@ def setUp(self): class ThreadSafeCleanupTestCase: """Subclass of unittest.TestCase with thread-safe cleanup methods. - This subclass protects the addCleanup() and doCleanups() methods - with a recursive lock. + This subclass protects the addCleanup() method with a recursive lock. + + doCleanups() is called when the server completed, but the client can still + be running in its thread especially if the server failed with a timeout. + Don't put a lock on doCleanups() to prevent deadlock between addCleanup() + called in the client and doCleanups() waiting for self.done.wait of + ThreadableTest._setUp() (gh-110167) """ def __init__(self, *args, **kwargs): @@ -230,9 +235,6 @@ def addCleanup(self, *args, **kwargs): with self._cleanup_lock: return super().addCleanup(*args, **kwargs) - def doCleanups(self, *args, **kwargs): - with self._cleanup_lock: - return super().doCleanups(*args, **kwargs) class SocketCANTest(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Tests/2023-10-05-19-33-49.gh-issue-110167.mIdj3v.rst b/Misc/NEWS.d/next/Tests/2023-10-05-19-33-49.gh-issue-110167.mIdj3v.rst new file mode 100644 index 00000000000000..d0cbbf9c3788bb --- /dev/null +++ b/Misc/NEWS.d/next/Tests/2023-10-05-19-33-49.gh-issue-110167.mIdj3v.rst @@ -0,0 +1,5 @@ +Fix a deadlock in test_socket when server fails with a timeout but the +client is still running in its thread. Don't hold a lock to call cleanup +functions in doCleanups(). One of the cleanup function waits until the +client completes, whereas the client could deadlock if it called +addCleanup() in such situation. Patch by Victor Stinner.