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

[Jun Wu]

Excerpts from Yuya Nishihara's message of 2016-07-16 01:02:51 +0900:
> 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().

That makes sense. It seems hard to change the architecture. Although if we
really want to, we can pass the file object.

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

Then how about "_runworker"? Anyway I don't feel strong as we don't need to
care about subclassing now.