bpo-35017: BaseServer should not accept any request after shutdown (GH-9952)

beledouxdenis · beledouxdenis · commit 57f0822397fb · 2018-10-25T13:59:55.000+02:00
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.
diff --git 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
@@ -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
@@ -0,0 +1,2 @@
+After shutdown, :class:`BaseServer` accepted a last request if it was sent
+between the socket polling and the polling timeout.

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+After shutdown, :class:`BaseServer` accepted a last request if it was sent
	`2`	`+between the socket polling and the polling timeout.`