[PATCH 3 of 7] commandserver: add new forking server implemented without using SocketServer
quark at fb.com
Fri Jul 15 12:29:50 EDT 2016
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.
More information about the Mercurial-devel