[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