Patchwork chgserver: auto exit after being idle for too long or lose the socket file

login
register
mail settings
Submitter Jun Wu
Date Feb. 16, 2016, 12:45 p.m.
Message ID <0517cfa5f688e58f397d.1455626712@x1c>
Download mbox | patch
Permalink /patch/13226/
State Superseded
Commit 0a853dc9b306ea8bbf8f1ebc6d6cf99c031e0f03
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 16, 2016, 12:45 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1455626546 0
#      Tue Feb 16 12:42:26 2016 +0000
# Node ID 0517cfa5f688e58f397d9384e4ace6cf2ed5250b
# Parent  9c6e0f51341bbf5461c199e6f0d1189af083bf46
chgserver: auto exit after being idle for too long or lose the socket file

This is a part of the one server per config series. In multiple-server setup,
new server may be started for a temporary config change like in command line,
--config extensions.foo=bar.py. This may end up with a lot of not so useful
server processes. Other questions are about socket file and process management,
How to stop these processes? What if a new server wants to listen on a same
address, replacing the old one?

This patch introduces AutoExitMixIn, which will:

1. Exit after being idle for too long. So useless servers won't run forever.
2. Periodically check the ownership of socket file, exit if it is no longer
   owned. This brings strong consistency between the filesystem and the
   process, and handles some race conditions neatly.
   Since rename is atomic, a new server can just have a same server address
   with an old one and won't worry about how to make sure the old server is
   killed, address conflict, service downtime issues.
   The user can safely stop all servers by simply removing the socket files,
   without worrying about outdated or accidentally removed pidfiles.
Yuya Nishihara - Feb. 24, 2016, 2:07 p.m.
On Tue, 16 Feb 2016 12:45:12 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1455626546 0
> #      Tue Feb 16 12:42:26 2016 +0000
> # Node ID 0517cfa5f688e58f397d9384e4ace6cf2ed5250b
> # Parent  9c6e0f51341bbf5461c199e6f0d1189af083bf46
> chgserver: auto exit after being idle for too long or lose the socket file
> 
> This is a part of the one server per config series. In multiple-server setup,
> new server may be started for a temporary config change like in command line,
> --config extensions.foo=bar.py. This may end up with a lot of not so useful
> server processes. Other questions are about socket file and process management,
> How to stop these processes? What if a new server wants to listen on a same
> address, replacing the old one?
> 
> This patch introduces AutoExitMixIn, which will:
> 
> 1. Exit after being idle for too long. So useless servers won't run forever.
> 2. Periodically check the ownership of socket file, exit if it is no longer
>    owned. This brings strong consistency between the filesystem and the
>    process, and handles some race conditions neatly.
>    Since rename is atomic, a new server can just have a same server address
>    with an old one and won't worry about how to make sure the old server is
>    killed, address conflict, service downtime issues.
>    The user can safely stop all servers by simply removing the socket files,
>    without worrying about outdated or accidentally removed pidfiles.
> 
> diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> --- a/hgext/chgserver.py
> +++ b/hgext/chgserver.py
> @@ -34,6 +34,8 @@
>  import re
>  import signal
>  import struct
> +import threading
> +import time
>  import traceback
>  
>  from mercurial.i18n import _
> @@ -382,20 +384,94 @@
>              traceback.print_exc(file=sv.cerr)
>              raise
>  
> +def _tempaddress(address):
> +    return '%s.%d.tmp' % (address, os.getpid())
> +
> +class AutoExitMixIn:
> +    lastactive = time.time()
> +    idletimeout = 3600  # defualt 1 hour
> +
> +    def startautoexitthread(self):
> +        # Note: The auto-exit check here is cheap enough to not use a thread,
> +        # but do it in serve_forever. However SocketServer is hook-unfriendly,
> +        # you simply cannot hook serve_forever without copying a lot of code.
> +        # Besides, serve_forever's docstring suggests using thread.
> +        thread = threading.Thread(target=self._autoexitloop)
> +        thread.daemon = True
> +        thread.start()
> +
> +    def _autoexitloop(self, interval=1):
> +        while True:
> +            time.sleep(interval)
> +            if not self.issocketowner():
> +                _log('%s is not owned, exiting.\n' % self.server_address)
> +                break
> +            if time.time() - self.lastactive > self.idletimeout:
> +                _log('being idle too long. exiting.\n')
> +                break
> +        self.shutdown()

I noticed _log() doesn't work in the main process, but that's okay for now.

> +    def server_bind(self):
> +        # use a unique temp address so we can stat the file and do ownership
> +        # check later
> +        tempaddress = _tempaddress(self.server_address)
> +        if self.allow_reuse_address:
> +            self.socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)

undefined name 'socket'.

Since we don't need it, I prefer not copying these lines from TCPServer.

> +        # rename will replace the old socket file if exists atomically. the
> +        # old server will detect ownership change and exit.
> +        os.rename(tempaddress, self.server_address)

Use util.rename() if possible, though chg never run on Windows.

> +    def unlinksocketfile(self):
> +        if not self.issocketowner():
> +            return
> +        # it is possible to have a race condition here that we may
> +        # remove another server's socket file. but that's okay
> +        # since that server will detect and exit automatically and
> +        # the client will start a new server on demand.
> +        try:
> +            os.unlink(self.server.server_address)

no attribute 'self.server'.

> +        except OSError:
> +            pass

Perhaps we'd better to raise the exception if errno != ENOENT.
Jun Wu - Feb. 24, 2016, 3:18 p.m.
On 02/24/2016 02:07 PM, Yuya Nishihara wrote:
> I noticed _log() doesn't work in the main process, but that's okay
> for now.
I always def _log while debugging and remove it before sending patches.
Will fix it later.

> undefined name 'socket'.

> no attribute 'self.server'.
Sorry for these. Are you checking them manually or using some static
analysis tool? It seems flake8 won't catch them.
Yuya Nishihara - Feb. 24, 2016, 3:48 p.m.
On Wed, 24 Feb 2016 15:18:13 +0000, Jun Wu wrote:
> On 02/24/2016 02:07 PM, Yuya Nishihara wrote:
> > undefined name 'socket'.

This one was caught by static analysis. Perhaps pyflakes or flake8?
You can also see it by test-check-pyflakes.t.

> > no attribute 'self.server'.

Got it by running the server.

> Sorry for these. Are you checking them manually or using some static
> analysis tool? It seems flake8 won't catch them.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -34,6 +34,8 @@ 
 import re
 import signal
 import struct
+import threading
+import time
 import traceback
 
 from mercurial.i18n import _
@@ -382,20 +384,94 @@ 
             traceback.print_exc(file=sv.cerr)
             raise
 
+def _tempaddress(address):
+    return '%s.%d.tmp' % (address, os.getpid())
+
+class AutoExitMixIn:
+    lastactive = time.time()
+    idletimeout = 3600  # defualt 1 hour
+
+    def startautoexitthread(self):
+        # Note: The auto-exit check here is cheap enough to not use a thread,
+        # but do it in serve_forever. However SocketServer is hook-unfriendly,
+        # you simply cannot hook serve_forever without copying a lot of code.
+        # Besides, serve_forever's docstring suggests using thread.
+        thread = threading.Thread(target=self._autoexitloop)
+        thread.daemon = True
+        thread.start()
+
+    def _autoexitloop(self, interval=1):
+        while True:
+            time.sleep(interval)
+            if not self.issocketowner():
+                _log('%s is not owned, exiting.\n' % self.server_address)
+                break
+            if time.time() - self.lastactive > self.idletimeout:
+                _log('being idle too long. exiting.\n')
+                break
+        self.shutdown()
+
+    def process_request(self, request, address):
+        self.lastactive = time.time()
+        return SocketServer.ForkingMixIn.process_request(
+            self, request, address)
+
+    def server_bind(self):
+        # use a unique temp address so we can stat the file and do ownership
+        # check later
+        tempaddress = _tempaddress(self.server_address)
+        if self.allow_reuse_address:
+            self.socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+        self.socket.bind(tempaddress)
+        self._socketstat = os.stat(tempaddress)
+        # rename will replace the old socket file if exists atomically. the
+        # old server will detect ownership change and exit.
+        os.rename(tempaddress, self.server_address)
+
+    def issocketowner(self):
+        try:
+            stat = os.stat(self.server_address)
+            return stat.st_ino == self._socketstat.st_ino and \
+                    stat.st_mtime == self._socketstat.st_mtime
+        except OSError:
+            return False
+
+    def unlinksocketfile(self):
+        if not self.issocketowner():
+            return
+        # it is possible to have a race condition here that we may
+        # remove another server's socket file. but that's okay
+        # since that server will detect and exit automatically and
+        # the client will start a new server on demand.
+        try:
+            os.unlink(self.server.server_address)
+        except OSError:
+            pass
+
 class chgunixservice(commandserver.unixservice):
     def init(self):
         # drop options set for "hg serve --cmdserver" command
         self.ui.setconfig('progress', 'assume-tty', None)
         signal.signal(signal.SIGHUP, self._reloadconfig)
-        class cls(SocketServer.ForkingMixIn, SocketServer.UnixStreamServer):
+        class cls(AutoExitMixIn, SocketServer.ForkingMixIn,
+                  SocketServer.UnixStreamServer):
             ui = self.ui
             repo = self.repo
         self.server = cls(self.address, _requesthandler)
+        self.server.idletimeout = self.ui.configint(
+            'chgserver', 'idletimeout', self.server.idletimeout)
+        self.server.startautoexitthread()
         # avoid writing "listening at" message to stdout before attachio
         # request, which calls setvbuf()
 
     def _reloadconfig(self, signum, frame):
         self.ui = self.server.ui = _renewui(self.ui)
 
+    def run(self):
+        try:
+            self.server.serve_forever()
+        finally:
+            self.server.unlinksocketfile()
+
 def uisetup(ui):
     commandserver._servicemap['chgunix'] = chgunixservice