[PATCH 1 of 4 V2] chgcache: implement socketpair-based IPC

[Yuya Nishihara]

On Mon, 27 Feb 2017 09:49:01 -0800, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-02-28 00:37:04 +0900:
> > On Wed, 22 Feb 2017 18:16:08 -0800, Jun Wu wrote:
> > > # HG changeset patch
> > > # User Jun Wu <quark at fb.com>
> > > # Date 1487802520 28800
> > > #      Wed Feb 22 14:28:40 2017 -0800
> > > # Node ID aef9e96fb573b85f5731367a470f574dbe730839
> > > # Parent  80f04ba7f4d1f439d726068f02172f9a52b981fe
> > > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r aef9e96fb573
> > > chgcache: implement socketpair-based IPC

> > > +    def recv(self):
> > > +        """receive a complete msg. will block."""
> > > +        select.select([self._in], [], [])
> > > +        # get message length, see "man tty_ioctl", not POSIX compliant
> > > +        intbuf = array.array('i', [0])
> > > +        fcntl.ioctl(self._in, termios.FIONREAD, intbuf)
> > > +        msglen = intbuf[0]
> > > +        # allocate one more byte, so we can detect bad msglen (bad OS)
> > > +        msg = self._in.recv(msglen + 1)
> > > +        assert len(msg) == msglen
> > > +        return msg
> > 
> > Looks okay, but can't we simply call recv() with reasonably large buffer size
> > (e.g. 8k) ?
> 
> That's actually a good idea. I was not very comfortable with the non-POSIX
> API too.
> 
> > Nit: if all peer ends were closed appropriately, recv() would return '' and the
> > assertion would fail.
> 
> Currently, that's impossible - the master does not close the fds.

So the assertion looked incorrect in point of socketipc class design.

> > > +    def __del__(self):
> > > +        self._in.close()
> > > +        self._out.close()
> > 
> > It's generally a bad idea to free resources by GC. Can't we have .close()
> > method?
> 
> Actually I had tried but it seems over complicated. "close" and "recv" may
> be called from different threads, thus thread lock may be necessary.

Maybe that's because the socketipc class manages channels that should actually
be separated. Producers only need the _out channel, and the consumer does _in.

(Strictly speaking, the _in channel should be closed in child processes after
fork(), but I don't care much about it since child processes are short-lived.)

> And it
> requires a way to interrupt a blocking "recv" from "close", which is quite
> painful to do "correctly" (see the reply to that patch).

Yep. If we aren't going to handle resources strictly, I would drop these
close() calls at all.

> It seems shm does
> not introduce these kinds of troubles.