From 365ef2f12d27f78909c75df7cf28d56517241556 Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Tue, 3 Nov 2020 09:03:36 -0500 Subject: [PATCH 1/5] Revert "Revert "bpo-37193: remove thread objects which finished process its request (GH-13893)" (GH-23107)" This reverts commit aca67da4fe68d5420401ac1782203d302875eb27. --- Lib/socketserver.py | 73 +++++++++++++++---- Lib/test/test_socketserver.py | 24 ++++++ .../2020-06-12-21-23-20.bpo-37193.wJximU.rst | 2 + 3 files changed, 86 insertions(+), 13 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2020-06-12-21-23-20.bpo-37193.wJximU.rst diff --git a/Lib/socketserver.py b/Lib/socketserver.py index 57c1ae6e9e8be1..6859b69682e972 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -128,6 +128,7 @@ class will essentially render the service "deaf" while one request is import os import sys import threading +import contextlib from io import BufferedIOBase from time import monotonic as time @@ -628,6 +629,55 @@ def server_close(self): self.collect_children(blocking=self.block_on_close) +class _Threads(list): + """ + Joinable list of all non-daemon threads. + """ + def __init__(self): + self._lock = threading.Lock() + + def append(self, thread): + if thread.daemon: + return + with self._lock: + super().append(thread) + + def remove(self, thread): + with self._lock: + # should not happen, but safe to ignore + with contextlib.suppress(ValueError): + super().remove(thread) + + def remove_current(self): + """Remove a current non-daemon thread.""" + thread = threading.current_thread() + if not thread.daemon: + self.remove(thread) + + def pop_all(self): + with self._lock: + self[:], result = [], self[:] + return result + + def join(self): + for thread in self.pop_all(): + thread.join() + + +class _NoThreads: + """ + Degenerate version of _Threads. + """ + def append(self, thread): + pass + + def join(self): + pass + + def remove_current(self): + pass + + class ThreadingMixIn: """Mix-in class to handle each request in a new thread.""" @@ -636,9 +686,9 @@ class ThreadingMixIn: daemon_threads = False # If true, server_close() waits until all non-daemonic threads terminate. block_on_close = True - # For non-daemonic threads, list of threading.Threading objects + # Threads object # used by server_close() to wait for all threads completion. - _threads = None + _threads = _NoThreads() def process_request_thread(self, request, client_address): """Same as in BaseServer but as a thread. @@ -651,27 +701,24 @@ def process_request_thread(self, request, client_address): except Exception: self.handle_error(request, client_address) finally: - self.shutdown_request(request) + try: + self.shutdown_request(request) + finally: + self._threads.remove_current() def process_request(self, request, client_address): """Start a new thread to process the request.""" + if self.block_on_close: + vars(self).setdefault('_threads', _Threads()) t = threading.Thread(target = self.process_request_thread, args = (request, client_address)) t.daemon = self.daemon_threads - if not t.daemon and self.block_on_close: - if self._threads is None: - self._threads = [] - self._threads.append(t) + self._threads.append(t) t.start() def server_close(self): super().server_close() - if self.block_on_close: - threads = self._threads - self._threads = None - if threads: - for thread in threads: - thread.join() + self._threads.join() if hasattr(os, "fork"): diff --git a/Lib/test/test_socketserver.py b/Lib/test/test_socketserver.py index 7cdd115a951539..1944795f058946 100644 --- a/Lib/test/test_socketserver.py +++ b/Lib/test/test_socketserver.py @@ -277,6 +277,13 @@ class MyHandler(socketserver.StreamRequestHandler): t.join() s.server_close() + def test_close_immediately(self): + class MyServer(socketserver.ThreadingMixIn, socketserver.TCPServer): + pass + + server = MyServer((HOST, 0), lambda: None) + server.server_close() + def test_tcpserver_bind_leak(self): # Issue #22435: the server socket wouldn't be closed if bind()/listen() # failed. @@ -491,6 +498,23 @@ def shutdown_request(self, request): self.assertEqual(server.shutdown_called, 1) server.server_close() + def test_threads_reaped(self): + """ + In #37193, users reported a memory leak + due to the saving of every request thread. Ensure that the + threads are cleaned up after the requests complete. + """ + class MyServer(socketserver.ThreadingMixIn, socketserver.TCPServer): + pass + + server = MyServer((HOST, 0), socketserver.StreamRequestHandler) + for n in range(10): + with socket.create_connection(server.server_address): + server.handle_request() + [thread.join() for thread in server._threads] + self.assertEqual(len(server._threads), 0) + server.server_close() + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Library/2020-06-12-21-23-20.bpo-37193.wJximU.rst b/Misc/NEWS.d/next/Library/2020-06-12-21-23-20.bpo-37193.wJximU.rst new file mode 100644 index 00000000000000..fbf56d3194cd22 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-06-12-21-23-20.bpo-37193.wJximU.rst @@ -0,0 +1,2 @@ +Fixed memory leak in ``socketserver.ThreadingMixIn`` introduced in Python +3.7. From 97c70544c9b0317b4d98cfdfa84454dc5369fb91 Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Tue, 3 Nov 2020 20:31:32 -0500 Subject: [PATCH 2/5] Reap dead threads when opening a new one. --- Lib/socketserver.py | 28 +++++++++++----------------- Lib/test/test_socketserver.py | 7 +++---- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/Lib/socketserver.py b/Lib/socketserver.py index 6859b69682e972..dbf87c691c9e8b 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -642,18 +642,6 @@ def append(self, thread): with self._lock: super().append(thread) - def remove(self, thread): - with self._lock: - # should not happen, but safe to ignore - with contextlib.suppress(ValueError): - super().remove(thread) - - def remove_current(self): - """Remove a current non-daemon thread.""" - thread = threading.current_thread() - if not thread.daemon: - self.remove(thread) - def pop_all(self): with self._lock: self[:], result = [], self[:] @@ -663,6 +651,14 @@ def join(self): for thread in self.pop_all(): thread.join() + def reap(self): + with self._lock: + dead = [thread for thread in self if not thread.is_alive()] + for thread in dead: + # should not happen, but safe to ignore + with contextlib.suppress(ValueError): + self.remove(thread) + class _NoThreads: """ @@ -674,7 +670,7 @@ def append(self, thread): def join(self): pass - def remove_current(self): + def reap(self): pass @@ -701,15 +697,13 @@ def process_request_thread(self, request, client_address): except Exception: self.handle_error(request, client_address) finally: - try: - self.shutdown_request(request) - finally: - self._threads.remove_current() + self.shutdown_request(request) def process_request(self, request, client_address): """Start a new thread to process the request.""" if self.block_on_close: vars(self).setdefault('_threads', _Threads()) + self._threads.reap() t = threading.Thread(target = self.process_request_thread, args = (request, client_address)) t.daemon = self.daemon_threads diff --git a/Lib/test/test_socketserver.py b/Lib/test/test_socketserver.py index 1944795f058946..954e0331352fb5 100644 --- a/Lib/test/test_socketserver.py +++ b/Lib/test/test_socketserver.py @@ -501,8 +501,8 @@ def shutdown_request(self, request): def test_threads_reaped(self): """ In #37193, users reported a memory leak - due to the saving of every request thread. Ensure that the - threads are cleaned up after the requests complete. + due to the saving of every request thread. Ensure that + not all threads are kept forever. """ class MyServer(socketserver.ThreadingMixIn, socketserver.TCPServer): pass @@ -511,8 +511,7 @@ class MyServer(socketserver.ThreadingMixIn, socketserver.TCPServer): for n in range(10): with socket.create_connection(server.server_address): server.handle_request() - [thread.join() for thread in server._threads] - self.assertEqual(len(server._threads), 0) + self.assertLess(len(server._threads), 10) server.server_close() From 8066645b12cc2530ed94ec59b2acff997ebb2c6e Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Wed, 4 Nov 2020 02:56:04 -0500 Subject: [PATCH 3/5] Now all _Threads operations occur in the main thread, so lock is unnecessary. --- Lib/socketserver.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/Lib/socketserver.py b/Lib/socketserver.py index dbf87c691c9e8b..85128400fc9c96 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -633,18 +633,13 @@ class _Threads(list): """ Joinable list of all non-daemon threads. """ - def __init__(self): - self._lock = threading.Lock() - def append(self, thread): if thread.daemon: return - with self._lock: - super().append(thread) + super().append(thread) def pop_all(self): - with self._lock: - self[:], result = [], self[:] + self[:], result = [], self[:] return result def join(self): @@ -652,12 +647,11 @@ def join(self): thread.join() def reap(self): - with self._lock: - dead = [thread for thread in self if not thread.is_alive()] - for thread in dead: - # should not happen, but safe to ignore - with contextlib.suppress(ValueError): - self.remove(thread) + dead = [thread for thread in self if not thread.is_alive()] + for thread in dead: + # should not happen, but safe to ignore + with contextlib.suppress(ValueError): + self.remove(thread) class _NoThreads: From 848ac2e29ebe0decf06a09b707c44e5083c5e50e Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Wed, 4 Nov 2020 03:00:38 -0500 Subject: [PATCH 4/5] Simplify reap --- Lib/socketserver.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Lib/socketserver.py b/Lib/socketserver.py index 85128400fc9c96..9176e2610da0a9 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -128,7 +128,6 @@ class will essentially render the service "deaf" while one request is import os import sys import threading -import contextlib from io import BufferedIOBase from time import monotonic as time @@ -647,11 +646,7 @@ def join(self): thread.join() def reap(self): - dead = [thread for thread in self if not thread.is_alive()] - for thread in dead: - # should not happen, but safe to ignore - with contextlib.suppress(ValueError): - self.remove(thread) + self[:] = (thread for thread in self if thread.is_alive()) class _NoThreads: From ec8e6895a3ce9cd69b6ceb75a15fcc74d4a522dc Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Wed, 4 Nov 2020 03:12:39 -0500 Subject: [PATCH 5/5] In _Threads, reap on append. --- Lib/socketserver.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Lib/socketserver.py b/Lib/socketserver.py index 9176e2610da0a9..0d9583d56a4d74 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -633,6 +633,7 @@ class _Threads(list): Joinable list of all non-daemon threads. """ def append(self, thread): + self.reap() if thread.daemon: return super().append(thread) @@ -659,9 +660,6 @@ def append(self, thread): def join(self): pass - def reap(self): - pass - class ThreadingMixIn: """Mix-in class to handle each request in a new thread.""" @@ -692,7 +690,6 @@ def process_request(self, request, client_address): """Start a new thread to process the request.""" if self.block_on_close: vars(self).setdefault('_threads', _Threads()) - self._threads.reap() t = threading.Thread(target = self.process_request_thread, args = (request, client_address)) t.daemon = self.daemon_threads