From 4cc06106799dafc6b525e433a5d12f61445cce88 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Thu, 29 Oct 2015 20:46:43 +0000 Subject: [PATCH] Remove serve_forever2/shutdown2 (reduces probability of #1085). I'm not even sure why `serve_forever2` and `shutdown2` were introduced in the first place... It probably follows from my misconception about the SocketServer module. After having studied the module again, I come to the conclusion that we can get rid of my crap, simultanously reducing probability of #1085 (hopefully down to 0)! `server_forever` is used throughout tests instead of `handle_request`, because `shutdown`, following docs, "must be called while serve_forever() is running in another thread, or it will deadlock", and our `probe_sni` HTTP request is already enough to kill single `handle_request`. We don't need to use any busy waiting block or `sleep` between serve and shutdown; studying CPython source code leads to the conclusion that the following construction is non-blocking: ```python import threading, SocketServer s = SocketServer.TCPServer(("", 0), None) t = threading.Thread(target=s.shutdown) t.start() s.serve_forever() # returns immediately t.join() # returns immediately ``` --- acme/acme/standalone.py | 28 ------------- acme/acme/standalone_test.py | 65 +++---------------------------- letsencrypt/plugins/standalone.py | 6 ++- 3 files changed, 10 insertions(+), 89 deletions(-) diff --git a/acme/acme/standalone.py b/acme/acme/standalone.py index 49759bc07..310e61995 100644 --- a/acme/acme/standalone.py +++ b/acme/acme/standalone.py @@ -4,7 +4,6 @@ import collections import functools import logging import os -import socket import sys import six @@ -50,37 +49,11 @@ class ACMEServerMixin: # pylint: disable=old-style-class server_version = "ACME client standalone challenge solver" allow_reuse_address = True - def __init__(self): - self._stopped = False - - def serve_forever2(self): - """Serve forever, until other thread calls `shutdown2`.""" - logger.debug("Starting server at %s:%d...", - *self.socket.getsockname()[:2]) - while not self._stopped: - self.handle_request() - - def shutdown2(self): - """Shutdown server loop from `serve_forever2`.""" - self._stopped = True - - # dummy request to terminate last server_forever2.handle_request() - sock = socket.socket() - try: - sock.connect(self.socket.getsockname()) - except socket.error: - pass # thread is probably already finished - finally: - sock.close() - - self.server_close() - class DVSNIServer(TLSServer, ACMEServerMixin): """DVSNI Server.""" def __init__(self, server_address, certs): - ACMEServerMixin.__init__(self) TLSServer.__init__( self, server_address, socketserver.BaseRequestHandler, certs=certs) @@ -89,7 +62,6 @@ class SimpleHTTPServer(BaseHTTPServer.HTTPServer, ACMEServerMixin): """SimpleHTTP Server.""" def __init__(self, server_address, resources): - ACMEServerMixin.__init__(self) BaseHTTPServer.HTTPServer.__init__( self, server_address, SimpleHTTPRequestHandler.partial_init( simple_http_resources=resources)) diff --git a/acme/acme/standalone_test.py b/acme/acme/standalone_test.py index 349581a3d..ed015b826 100644 --- a/acme/acme/standalone_test.py +++ b/acme/acme/standalone_test.py @@ -1,7 +1,6 @@ """Tests for acme.standalone.""" import os import shutil -import socket import threading import tempfile import time @@ -29,54 +28,6 @@ class TLSServerTest(unittest.TestCase): server.server_close() # pylint: disable=no-member -class ACMEServerMixinTest(unittest.TestCase): - """Tests for acme.standalone.ACMEServerMixin.""" - - def setUp(self): - from acme.standalone import ACMEServerMixin - - class _MockHandler(socketserver.BaseRequestHandler): - # pylint: disable=missing-docstring,no-member,no-init - - def handle(self): - self.request.sendall(b"DONE") - - class _MockServer(socketserver.TCPServer, ACMEServerMixin): - def __init__(self, *args, **kwargs): - socketserver.TCPServer.__init__(self, *args, **kwargs) - ACMEServerMixin.__init__(self) - - self.server = _MockServer(("", 0), _MockHandler) - - def _busy_wait(self): # pragma: no cover - # This function is used to avoid race conditions in tests, but - # not all of the functionality is always used, hence "no - # cover" - while True: - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - try: - # pylint: disable=no-member - sock.connect(self.server.socket.getsockname()) - except socket.error: - pass - else: - sock.recv(4) # wait until handle_request is actually called - break - finally: - sock.close() - time.sleep(1) - - def test_serve_shutdown(self): - thread = threading.Thread(target=self.server.serve_forever2) - thread.start() - self._busy_wait() - self.server.shutdown2() - - def test_shutdown2_not_running(self): - self.server.shutdown2() - self.server.shutdown2() - - class DVSNIServerTest(unittest.TestCase): """Test for acme.standalone.DVSNIServer.""" @@ -89,20 +40,16 @@ class DVSNIServerTest(unittest.TestCase): from acme.standalone import DVSNIServer self.server = DVSNIServer(("", 0), certs=self.certs) # pylint: disable=no-member - self.thread = threading.Thread(target=self.server.handle_request) + self.thread = threading.Thread(target=self.server.serve_forever) self.thread.start() def tearDown(self): - self.server.shutdown2() + self.server.shutdown() # pylint: disable=no-member self.thread.join() - def test_init(self): - # pylint: disable=protected-access - self.assertFalse(self.server._stopped) - - def test_dvsni(self): + def test_it(self): host, port = self.server.socket.getsockname()[:2] - cert = crypto_util.probe_sni(b'localhost', host=host, port=port) + cert = crypto_util.probe_sni(b'localhost', host=host, port=port, timeout=1) self.assertEqual(jose.ComparableX509(cert), jose.ComparableX509(self.certs[b'localhost'][1])) @@ -120,11 +67,11 @@ class SimpleHTTPServerTest(unittest.TestCase): # pylint: disable=no-member self.port = self.server.socket.getsockname()[1] - self.thread = threading.Thread(target=self.server.handle_request) + self.thread = threading.Thread(target=self.server.serve_forever) self.thread.start() def tearDown(self): - self.server.shutdown2() + self.server.shutdown() # pylint: disable=no-member self.thread.join() def test_index(self): diff --git a/letsencrypt/plugins/standalone.py b/letsencrypt/plugins/standalone.py index 1b56fbdbc..ccff03319 100644 --- a/letsencrypt/plugins/standalone.py +++ b/letsencrypt/plugins/standalone.py @@ -72,7 +72,9 @@ class ServerManager(object): except socket.error as error: raise errors.StandaloneBindError(error, port) - thread = threading.Thread(target=server.serve_forever2) + thread = threading.Thread( + # pylint: disable=no-member + target=server.serve_forever) thread.start() # if port == 0, then random free port on OS is taken @@ -90,7 +92,7 @@ class ServerManager(object): instance = self._instances[port] logger.debug("Stopping server at %s:%d...", *instance.server.socket.getsockname()[:2]) - instance.server.shutdown2() + instance.server.shutdown() instance.thread.join() del self._instances[port]