Make Python sync server stoppable

sync/tools/testserver/sync_testserver.py

 #!/usr/bin/env python
 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """This is a python sync server used for testing Chrome Sync.
 
 By default, it listens on an ephemeral port and xmpp_port and sends the port
 numbers back to the originating process over a pipe. The originating process can
 specify an explicit port and xmpp_port if necessary.
 """
 
 import asyncore
 import BaseHTTPServer
 import errno
 import os
 import select
 import socket
 import sys
 import urlparse
 
 import chromiumsync
 import echo_message
 import testserver_base
 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(
       self._xmpp_socket_map, ('localhost', xmpp_port))
     self.xmpp_port = self._xmpp_server.getsockname()[1]
     self.authenticated = True
 
   def GetXmppServer(self):
     return self._xmpp_server
 
@@ skipping 38 matching lines @@
       # removed from socket_map.
       if xmpp_connection is None:
         return
       try:
         handler(xmpp_connection)
       except (asyncore.ExitNow, KeyboardInterrupt, SystemExit):
         raise
       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 = []
 
       for fd, xmpp_connection in self._xmpp_socket_map.items():
         is_r = xmpp_connection.readable()
         is_w = xmpp_connection.writable()
         if is_r:
           read_fds.append(fd)
         if is_w:
@@ skipping 451 matching lines @@
     testserver_base.TestServerRunner.add_options(self)
     self.option_parser.add_option('--xmpp-port', default='0', type='int',
                                   help='Port used by the XMPP server. If '
                                   'unspecified, the XMPP server will listen on '
                                   'an ephemeral port.')
     # Override the default logfile name used in testserver.py.
     self.option_parser.set_defaults(log_file='sync_testserver.log')
 
 if __name__ == '__main__':
   sys.exit(SyncServerRunner().main())