diff --git a/Lib/socketserver.py b/Lib/socketserver.py index dacec0376bd..18aec290e89 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -198,7 +198,7 @@ class BaseServer: self.server_address = server_address self.RequestHandlerClass = RequestHandlerClass self.__is_shut_down = threading.Event() - self.__serving = False + self.__shutdown_request = False def server_activate(self): """Called by constructor to activate the server. @@ -215,17 +215,19 @@ class BaseServer: self.timeout. If you need to do periodic tasks, do them in another thread. """ - self.__serving = True self.__is_shut_down.clear() - while self.__serving: - # XXX: Consider using another file descriptor or - # connecting to the socket to wake this up instead of - # polling. Polling reduces our responsiveness to a - # shutdown request and wastes cpu at all other times. - r, w, e = select.select([self], [], [], poll_interval) - if r: - self._handle_request_noblock() - self.__is_shut_down.set() + try: + while not self.__shutdown_request: + # XXX: Consider using another file descriptor or + # connecting to the socket to wake this up instead of + # polling. Polling reduces our responsiveness to a + # shutdown request and wastes cpu at all other times. + r, w, e = select.select([self], [], [], poll_interval) + if self in r: + self._handle_request_noblock() + finally: + self.__shutdown_request = False + self.__is_shut_down.set() def shutdown(self): """Stops the serve_forever loop. @@ -234,7 +236,7 @@ class BaseServer: serve_forever() is running in another thread, or it will deadlock. """ - self.__serving = False + self.__shutdown_request = True self.__is_shut_down.wait() # The distinction between handling, getting, processing and diff --git a/Lib/test/test_socketserver.py b/Lib/test/test_socketserver.py index a38e0fd64f1..957848952df 100644 --- a/Lib/test/test_socketserver.py +++ b/Lib/test/test_socketserver.py @@ -241,6 +241,31 @@ class SocketServerTest(unittest.TestCase): # socketserver.DatagramRequestHandler, # self.dgram_examine) + @reap_threads + def test_shutdown(self): + # Issue #2302: shutdown() should always succeed in making an + # other thread leave serve_forever(). + class MyServer(socketserver.TCPServer): + pass + + class MyHandler(socketserver.StreamRequestHandler): + pass + + threads = [] + for i in range(20): + s = MyServer((HOST, 0), MyHandler) + t = threading.Thread( + name='MyServer serving', + target=s.serve_forever, + kwargs={'poll_interval':0.01}) + t.daemon = True # In case this function raises. + threads.append((t, s)) + for t, s in threads: + t.start() + s.shutdown() + for t, s in threads: + t.join() + def test_main(): if imp.lock_held(): diff --git a/Misc/NEWS b/Misc/NEWS index fb97f53df34..e72463a48e7 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -339,6 +339,11 @@ C-API Library ------- +- Issue #2302: Fix a race condition in SocketServer.BaseServer.shutdown, + where the method could block indefinitely if called just before the + event loop started running. This also fixes the occasional freezes + witnessed in test_httpservers. + - Issue #8524: When creating an SSL socket, the timeout value of the original socket wasn't retained (instead, a socket with a positive timeout would be turned into a non-blocking SSL socket).