Make Python sync server stoppable

sync/tools/testserver/sync_testserver.py

Index: sync/tools/testserver/sync_testserver.py
diff --git a/sync/tools/testserver/sync_testserver.py b/sync/tools/testserver/sync_testserver.py
index b3ca64203043cac0040294efc69ebd98b3f917d3..73ce93f5e11ca8c2d8335c0881248f8483ee4f64 100755
--- a/sync/tools/testserver/sync_testserver.py
+++ b/sync/tools/testserver/sync_testserver.py
@@ -27,13 +27,13 @@ import xmppserver
 
 class SyncHTTPServer(testserver_base.ClientRestrictingServerMixIn,

[maniscalco, 2015/01/06 18:39:59]

What's the overall goal of this change?  Does the current behavior prevent you
from writing a new test case?  Does the current behavior cause existing tests to
be flaky?  Or is this just general cleanup?

[sigbjorn, 2015/01/07 09:26:34]

This allows for clean shutdown of the server. On Linux test slaves, killing the
(binary) thread which started the server would not kill the sync server, leading
to leftover processes. The proper fix is to shut down the server cleanly,
instead of relying on killing processes.

                      testserver_base.BrokenPipeHandlerMixIn,
-                     testserver_base.StoppableHTTPServer):
+                     BaseHTTPServer.HTTPServer):
   """An HTTP server that handles sync commands."""
 
   def __init__(self, server_address, xmpp_port, request_handler_class):
-    testserver_base.StoppableHTTPServer.__init__(self,
-                                                 server_address,
-                                                 request_handler_class)
+    BaseHTTPServer.HTTPServer.__init__(self,
+                                       server_address,
+                                       request_handler_class)
     self._sync_handler = chromiumsync.TestServer()
     self._xmpp_socket_map = {}
     self._xmpp_server = xmppserver.XmppServer(
@@ -92,7 +92,8 @@ class SyncHTTPServer(testserver_base.ClientRestrictingServerMixIn,
       except:
         xmpp_connection.handle_error()
 
-    while True:
+    self.stop = False

[pval...(no longer on Chromium), 2015/01/05 17:49:01]

when/where can this be set to True?

[sigbjorn, 2015/01/06 08:11:27]

This would be set to True from the calling thread. In my example that triggered
this fix:

Startup:
self.mock_server = sync_testserver.SyncHTTPServer(('127.0.0.1', 0), 0,
sync_testserver.SyncPageHandler)	 
self._mock_server_thread =
threading.Thread(target=self.mock_server.serve_forever, name="SyncServerThread")

Shutdown:
self.mock_server.stop = True
# The server might not shut down instantaneously
if not self._opauto.WaitUntil(lambda: not self._mock_server_thread.isAlive(),
timeout=10):
  print "ERROR in teardown: Sync mock server failed to shut down. Continuing
with remaining teardown."

The similar code in the now-removed testserver_base.StoppableHTTPServer reads:
def serve_forever(self):
  self.stop = False
  self.nonce_time = None
  while not self.stop:
    self.handle_request()
  self.socket.close()

[maniscalco, 2015/01/06 18:39:59]

Maybe I'm missing something, why not create a Stop method?  Why reach into this
object and assign to stop?

Also, doesn't it seems like Stop should only return once the object has stopped?

[sigbjorn, 2015/01/07 09:26:34]

The reason is compatibility. This is what StoppableHTTPServer does, and thus
what this module already attempted (but failed) to do. This is what other Python
servers and code in the repository does (anything that inherits from
StoppableHTTPServer), as well as the standard Python way (e.g. the threading API
has deprecated getName/setName and isDaemon/setDaemon in favor of
assigning/reading straight to properties).
No, this is just a signal for the thread to shut down. The point of a thread is
that it doesn't block the caller, so only returning once the server is gone
would defeat part of the point of using a separate thread. If you want to block
the caller until the server is gone, you'd call join on the server thread after
setting .stop to True.

+    while not self.stop:
       read_fds = [ self.fileno() ]
       write_fds = []
       exceptional_fds = []