Skip to content

Commit aca67da

Browse files
authored
Revert "bpo-37193: remove thread objects which finished process its request (GH-13893)" (GH-23107)
This reverts commit c415590.
1 parent 4b9aad4 commit aca67da

File tree

3 files changed

+13
-86
lines changed

3 files changed

+13
-86
lines changed

Lib/socketserver.py

+13-60
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,6 @@ class will essentially render the service "deaf" while one request is
128128
import os
129129
import sys
130130
import threading
131-
import contextlib
132131
from io import BufferedIOBase
133132
from time import monotonic as time
134133

@@ -629,55 +628,6 @@ def server_close(self):
629628
self.collect_children(blocking=self.block_on_close)
630629

631630

632-
class _Threads(list):
633-
"""
634-
Joinable list of all non-daemon threads.
635-
"""
636-
def __init__(self):
637-
self._lock = threading.Lock()
638-
639-
def append(self, thread):
640-
if thread.daemon:
641-
return
642-
with self._lock:
643-
super().append(thread)
644-
645-
def remove(self, thread):
646-
with self._lock:
647-
# should not happen, but safe to ignore
648-
with contextlib.suppress(ValueError):
649-
super().remove(thread)
650-
651-
def remove_current(self):
652-
"""Remove a current non-daemon thread."""
653-
thread = threading.current_thread()
654-
if not thread.daemon:
655-
self.remove(thread)
656-
657-
def pop_all(self):
658-
with self._lock:
659-
self[:], result = [], self[:]
660-
return result
661-
662-
def join(self):
663-
for thread in self.pop_all():
664-
thread.join()
665-
666-
667-
class _NoThreads:
668-
"""
669-
Degenerate version of _Threads.
670-
"""
671-
def append(self, thread):
672-
pass
673-
674-
def join(self):
675-
pass
676-
677-
def remove_current(self):
678-
pass
679-
680-
681631
class ThreadingMixIn:
682632
"""Mix-in class to handle each request in a new thread."""
683633

@@ -686,9 +636,9 @@ class ThreadingMixIn:
686636
daemon_threads = False
687637
# If true, server_close() waits until all non-daemonic threads terminate.
688638
block_on_close = True
689-
# Threads object
639+
# For non-daemonic threads, list of threading.Threading objects
690640
# used by server_close() to wait for all threads completion.
691-
_threads = _NoThreads()
641+
_threads = None
692642

693643
def process_request_thread(self, request, client_address):
694644
"""Same as in BaseServer but as a thread.
@@ -701,24 +651,27 @@ def process_request_thread(self, request, client_address):
701651
except Exception:
702652
self.handle_error(request, client_address)
703653
finally:
704-
try:
705-
self.shutdown_request(request)
706-
finally:
707-
self._threads.remove_current()
654+
self.shutdown_request(request)
708655

709656
def process_request(self, request, client_address):
710657
"""Start a new thread to process the request."""
711-
if self.block_on_close:
712-
vars(self).setdefault('_threads', _Threads())
713658
t = threading.Thread(target = self.process_request_thread,
714659
args = (request, client_address))
715660
t.daemon = self.daemon_threads
716-
self._threads.append(t)
661+
if not t.daemon and self.block_on_close:
662+
if self._threads is None:
663+
self._threads = []
664+
self._threads.append(t)
717665
t.start()
718666

719667
def server_close(self):
720668
super().server_close()
721-
self._threads.join()
669+
if self.block_on_close:
670+
threads = self._threads
671+
self._threads = None
672+
if threads:
673+
for thread in threads:
674+
thread.join()
722675

723676

724677
if hasattr(os, "fork"):

Lib/test/test_socketserver.py

-24
Original file line numberDiff line numberDiff line change
@@ -277,13 +277,6 @@ class MyHandler(socketserver.StreamRequestHandler):
277277
t.join()
278278
s.server_close()
279279

280-
def test_close_immediately(self):
281-
class MyServer(socketserver.ThreadingMixIn, socketserver.TCPServer):
282-
pass
283-
284-
server = MyServer((HOST, 0), lambda: None)
285-
server.server_close()
286-
287280
def test_tcpserver_bind_leak(self):
288281
# Issue #22435: the server socket wouldn't be closed if bind()/listen()
289282
# failed.
@@ -498,23 +491,6 @@ def shutdown_request(self, request):
498491
self.assertEqual(server.shutdown_called, 1)
499492
server.server_close()
500493

501-
def test_threads_reaped(self):
502-
"""
503-
In #37193, users reported a memory leak
504-
due to the saving of every request thread. Ensure that the
505-
threads are cleaned up after the requests complete.
506-
"""
507-
class MyServer(socketserver.ThreadingMixIn, socketserver.TCPServer):
508-
pass
509-
510-
server = MyServer((HOST, 0), socketserver.StreamRequestHandler)
511-
for n in range(10):
512-
with socket.create_connection(server.server_address):
513-
server.handle_request()
514-
[thread.join() for thread in server._threads]
515-
self.assertEqual(len(server._threads), 0)
516-
server.server_close()
517-
518494

519495
if __name__ == "__main__":
520496
unittest.main()

Misc/NEWS.d/next/Library/2020-06-12-21-23-20.bpo-37193.wJximU.rst

-2
This file was deleted.

0 commit comments

Comments
 (0)