[PATCH 3 of 7] commandserver: add new forking server implemented without using SocketServer

Yuya Nishihara yuya at tcha.org
Fri Jul 15 12:02:51 EDT 2016


On Fri, 15 Jul 2016 16:27:56 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-07-15 00:20:41 +0900:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya at tcha.org>
> > # Date 1463884998 -32400
> > #      Sun May 22 11:43:18 2016 +0900
> > # Node ID e56508ea57c23af68d05d941d344321b6ade8184
> > # Parent  e374627f118e8aca6243d263154aab9309abf1fc
> > commandserver: add new forking server implemented without using SocketServer

> > +class unixservicehandler(object):
> > +    """Set of pluggable operations for unix-mode services
> > +
> > +    Almost all methods except for createcmdserver() are called in the main
> > +    process. You can't pass mutable resource back from createcmdserver().
> > +    """
> > +
> > +    pollinterval = None
> > +
> > +    def __init__(self, ui):
> > +        self.ui = ui  
> 
> Do we really need the ui object? I'm a big fan of avoiding "self.ui = ui"
> whenever possible.

Good question. I made it keep ui because future patches will replace _log(),
which is noop now, by ui.debug().

> > +    def createcmdserver(self, repo, conn, fin, fout):  
> 
> And add a ui parameter here so we can remove "self.ui = ui". As "repo" is
> here already, it will feel more consistent.

Yep, I initially designed it to accept (ui, repo, ...), but dropped ui because
of the reason above.

> > +class unixforkingservice(unixservice):
> > +    def __init__(self, ui, repo, opts, handler=None):
> > +        super(unixforkingservice, self).__init__(ui, repo, opts)
> > +        self._servicehandler = handler or unixservicehandler(ui)
> > +        self._sock = None
> > +        self._oldsigchldhandler = None
> > +        self._workerpids = set()  # updated by signal handler; do not iterate
> > +
> > +    def init(self):
> > +        self._sock = socket.socket(socket.AF_UNIX)
> > +        self._servicehandler.bindsocket(self._sock, self.address)
> > +        self._sock.listen(5)  
> 
> Maybe make 5 a class constant, or as we have ui, read from some config
> option. But that can be a later patch.

A class constant seems good. I'll do that if I need to revise this patch.

> > +    def _serveworker(self, conn):  
> 
> I find the name "_serveworker" a bit ambiguous. "serve" is usually a verb
> and iiuc it's used as a noun here. Maybe just use "_serverequest" or move it
> to mainloop (makes sense to me since it's simple and the fork logic is
> already there, and I think we don't care about subclass now).

I'm going to move the initialization of the worker process to this function,
which is why I avoid calling it _serverequest(). If we add a prefork server,
setpgid() and random.seed() shouldn't be executed per request, so they will
be moved from _serverequest() to unixforkingservice._serveworker().


More information about the Mercurial-devel mailing list