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