From 57f0822397fb78ff2540cd26476ecdb3933fc816 Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Thu, 18 Oct 2018 13:56:09 +0200 Subject: [PATCH 1/4] bpo-35017: `BaseServer` should not accept any request after shutdown (GH-9952) Prior to this revision, After the shutdown of a `BaseServer`, the server accepted a last single request if it was sent between the server socket polling and the polling timeout. This can be problematic for instance for a server restart for which you do not want to interrupt the service, by not closing the listening socket during the restart. One request failed because of this behavior. Note that only one request failed, following requests were not accepted, as expected. --- Lib/socketserver.py | 4 +++ Lib/test/test_socketserver.py | 31 +++++++++++++++++++ .../2018-10-19-09-32-07.bpo-35017.vAanVd.rst | 2 ++ 3 files changed, 37 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst diff --git a/Lib/socketserver.py b/Lib/socketserver.py index 9dfd21bab9b6f9..facdb939f4bfa9 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -230,6 +230,10 @@ def serve_forever(self, poll_interval=0.5): while not self.__shutdown_request: ready = selector.select(poll_interval) + # bpo-35017: request arriving after the server shutdown + # and before the poll_interval timeout should not be accepted. + if self.__shutdown_request: + break if ready: self._handle_request_noblock() diff --git a/Lib/test/test_socketserver.py b/Lib/test/test_socketserver.py index 6584ba5ba864e2..fd1e3e1288d8e8 100644 --- a/Lib/test/test_socketserver.py +++ b/Lib/test/test_socketserver.py @@ -492,6 +492,37 @@ def shutdown_request(self, request): self.assertEqual(server.shutdown_called, 1) server.server_close() + def test_no_accept_after_shutdown(self): + rejected = False + s = socketserver.TCPServer((HOST, 0), socketserver.StreamRequestHandler) + + thread_serve = threading.Thread( + name='MyServer serving', + target=s.serve_forever) + thread_serve.daemon = True # In case this function raises. + thread_serve.start() + + thread_shutdown = threading.Thread( + name='MyServer shutdown', + target=s.shutdown) + thread_shutdown.daemon = True # In case this function raises. + thread_shutdown.start() + + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.settimeout(0.1) + sock.connect(s.server_address) + sock.send(b"Request after shutdown\n") + try: + sock.recv(1024) + except socket.timeout: + rejected = True + sock.close() + + for t in [thread_serve, thread_shutdown]: + t.join() + s.server_close() + self.assertEqual(rejected, True) + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst b/Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst new file mode 100644 index 00000000000000..12b62c07c36c56 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst @@ -0,0 +1,2 @@ +After shutdown, :class:`BaseServer` accepted a last request if it was sent +between the socket polling and the polling timeout. From 928be3765b977b80a48d4144b0b055f6243b431d Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Fri, 26 Oct 2018 00:02:14 +0200 Subject: [PATCH 2/4] bpo-35017: remove the unit test as it's time-based This test can fail on slow buildbots, because it relies on a time condition The unit test can be safely removed, the fix being quite trivial. --- Lib/test/test_socketserver.py | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/Lib/test/test_socketserver.py b/Lib/test/test_socketserver.py index fd1e3e1288d8e8..6584ba5ba864e2 100644 --- a/Lib/test/test_socketserver.py +++ b/Lib/test/test_socketserver.py @@ -492,37 +492,6 @@ def shutdown_request(self, request): self.assertEqual(server.shutdown_called, 1) server.server_close() - def test_no_accept_after_shutdown(self): - rejected = False - s = socketserver.TCPServer((HOST, 0), socketserver.StreamRequestHandler) - - thread_serve = threading.Thread( - name='MyServer serving', - target=s.serve_forever) - thread_serve.daemon = True # In case this function raises. - thread_serve.start() - - thread_shutdown = threading.Thread( - name='MyServer shutdown', - target=s.shutdown) - thread_shutdown.daemon = True # In case this function raises. - thread_shutdown.start() - - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.settimeout(0.1) - sock.connect(s.server_address) - sock.send(b"Request after shutdown\n") - try: - sock.recv(1024) - except socket.timeout: - rejected = True - sock.close() - - for t in [thread_serve, thread_shutdown]: - t.join() - s.server_close() - self.assertEqual(rejected, True) - if __name__ == "__main__": unittest.main() From b9fb425f3e87ad8f52683708235e731654011a9d Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Fri, 26 Oct 2018 00:12:46 +0200 Subject: [PATCH 3/4] bpo-35017: reword the news to represent the new behavior The news message now represents what the revision is changing regarding the behavior rather than describing the current unexpected behavior. --- .../next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst | 2 -- .../next/Library/2018-10-26-00-11-21.bpo-35017.6Ez4Cv.rst | 3 +++ 2 files changed, 3 insertions(+), 2 deletions(-) delete mode 100644 Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst create mode 100644 Misc/NEWS.d/next/Library/2018-10-26-00-11-21.bpo-35017.6Ez4Cv.rst diff --git a/Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst b/Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst deleted file mode 100644 index 12b62c07c36c56..00000000000000 --- a/Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst +++ /dev/null @@ -1,2 +0,0 @@ -After shutdown, :class:`BaseServer` accepted a last request if it was sent -between the socket polling and the polling timeout. diff --git a/Misc/NEWS.d/next/Library/2018-10-26-00-11-21.bpo-35017.6Ez4Cv.rst b/Misc/NEWS.d/next/Library/2018-10-26-00-11-21.bpo-35017.6Ez4Cv.rst new file mode 100644 index 00000000000000..5682717adf70f9 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-10-26-00-11-21.bpo-35017.6Ez4Cv.rst @@ -0,0 +1,3 @@ +:meth:`socketserver.BaseServer.serve_forever` now exits immediately if it's +:meth:`~socketserver.BaseServer.shutdown` method is called while it is +polling for new events. From 7992fb06b57899b2ea84115e29d6ac64c89b8112 Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Fri, 26 Oct 2018 14:39:37 +0200 Subject: [PATCH 4/4] bpo-35017: reword inline comment to be shorter and cleaner --- Lib/socketserver.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/socketserver.py b/Lib/socketserver.py index facdb939f4bfa9..f0377918e89436 100644 --- a/Lib/socketserver.py +++ b/Lib/socketserver.py @@ -230,8 +230,7 @@ def serve_forever(self, poll_interval=0.5): while not self.__shutdown_request: ready = selector.select(poll_interval) - # bpo-35017: request arriving after the server shutdown - # and before the poll_interval timeout should not be accepted. + # bpo-35017: shutdown() called during select(), exit immediately. if self.__shutdown_request: break if ready: