Make Python sync server stoppable

Make Python sync server stoppable

The testserver attempts to inherit from StoppableHTTPServer, but then
overrides the stoppable functionality. Instead of overriding serve_forever, override handle_request, to maintain stoppability.

BUG=

Committed: https://crrev.com/1e630129a6dbb309618c97d0b63a1c42d689e679
Cr-Commit-Position: refs/heads/master@{#310732}

[sigbjorn, 2015-01-05 13:16:00]

sigbjorn@opera.com changed reviewers:
+ rsimha@chromium.org

[sigbjorn, 2015-01-05 13:17:25]

Raghu, you seem to have written this file, could you review this, or suggest a
more suitable reviewer?

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

pvalenzuela@chromium.org changed reviewers:
+ pvalenzuela@chromium.org

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

I'm happy to review this, but you'll need OWNERS approval from someone else.

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
File sync/tools/testserver/sync_testserver.py (right):

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
sync/tools/testserver/sync_testserver.py:95: self.stop = False
when/where can this be set to True?

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

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
File sync/tools/testserver/sync_testserver.py (right):

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
sync/tools/testserver/sync_testserver.py:95: self.stop = False
On 2015/01/05 17:49:01, pvalenzuela wrote:
> when/where can this be set to True?

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()

[pval...(no longer on Chromium), 2015-01-06 18:24:59]

pvalenzuela@chromium.org changed reviewers:
+ maniscalco@chromium.org
- rsimha@chromium.org

[pval...(no longer on Chromium), 2015-01-06 18:25:00]

lgtm

Raghu no longer works on Sync, so I'll let maniscalco@ provide the owner
approval.

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

I'm not too familiar with this code so bear with me...

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
File sync/tools/testserver/sync_testserver.py (right):

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
sync/tools/testserver/sync_testserver.py:28: class
SyncHTTPServer(testserver_base.ClientRestrictingServerMixIn,
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?

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
sync/tools/testserver/sync_testserver.py:95: self.stop = False
On 2015/01/06 08:11:27, sigbjorn wrote:
> On 2015/01/05 17:49:01, pvalenzuela wrote:
> > when/where can this be set to True?
> 
> 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()

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]

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
File sync/tools/testserver/sync_testserver.py (right):

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
sync/tools/testserver/sync_testserver.py:28: class
SyncHTTPServer(testserver_base.ClientRestrictingServerMixIn,
On 2015/01/06 18:39:59, maniscalco wrote:
> 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?

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.

https://codereview.chromium.org/797253003/diff/1/sync/tools/testserver/sync_t...
sync/tools/testserver/sync_testserver.py:95: self.stop = False
On 2015/01/06 18:39:59, maniscalco wrote:
> On 2015/01/06 08:11:27, sigbjorn wrote:
> > On 2015/01/05 17:49:01, pvalenzuela wrote:
> > > when/where can this be set to True?
> > 
> > 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()
> 
> Maybe I'm missing something, why not create a Stop method?  Why reach into
this
> object and assign to stop?

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).

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

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.

[pval...(no longer on Chromium), 2015-01-07 21:43:32]

sigbjorn@,

maniscalco@ and I discussed this change. We are wondering if it would be
possible to still use StoppableHTTPServer instead of re-inventing its stop
logic. For example, could we override handle_request() (called by
StoppableHTTPServer.serve_forever()) and then revert back to using
StoppableHTTPServer?

[sigbjorn, 2015-01-08 09:06:38]

(Don't think you guys get a mail unless I make an extraneous comment here.)
Patch set 2 maintains the inheritance, and changes which function is overridden
instead.

[pval...(no longer on Chromium), 2015-01-08 18:41:38]

lgtm

Cool! This is exactly what I had in mind; I think it's cleaner.

Do sync_integration_tests still work fine? I assume one of the try jobs picked
it up.

[maniscalco, 2015-01-08 20:28:34]

On 2015/01/08 18:41:38, pvalenzuela wrote:
> lgtm
> 
> Cool! This is exactly what I had in mind; I think it's cleaner.
> 
> Do sync_integration_tests still work fine? I assume one of the try jobs picked
> it up.

LGTM (assuming sync_integration_tests still pass)

[sigbjorn, 2015-01-09 07:58:52]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2015-01-09 07:59:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797253003/20001

[commit-bot: I haz the power, 2015-01-09 08:02:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-09 08:03:44]

Patchset 2 (id:??) landed as
https://crrev.com/1e630129a6dbb309618c97d0b63a1c42d689e679
Cr-Commit-Position: refs/heads/master@{#310732}