Re: [Tarantool-patches] [PATCH v10 2/3] popen: introduce a backend engine

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Mon, Feb 17, 2020 at 12:11:47PM +0300, Cyrill Gorcunov wrote:
> On Mon, Feb 17, 2020 at 02:04:42AM +0300, Alexander Turenko wrote:
> > It seems I constantly have no time to concentrate on this nice feature.
> > 
> > I would however share my fear about it: libev doing a lot of work to
> > make epoll() work properly. See [1], EVBACKEND_EPOLL description.
> > 
> > For example,
> > 
> >  | The biggest issue is fork races, however - if a program forks then
> >  | both parent and child process have to recreate the epoll set, which
> >  | can take considerable time (one syscall per file descriptor) and is
> >  | of course hard to detect.
> > 
> > Is it applicable for vfork()?
> 
> Yes. vfork very similar to fork internally. Though I don't get what
> he means by "(one syscall per file descriptor)". The kernel doesn't
> do additional syscalls except vfork itsel. The kernel simply allocates
> new fd table and copies old fd into it. This is time consuming of
> course and more importantly useless from our point of view since
> we do exec pretty soon. But we simply have no other mechanism to use.
> 
> > I would feel much more comfortable if we would look though libev docs /
> > code to at least be aware about such possibilities. After this we can
> > say, whether popen engine is safe comparing to libev (which should be
> > good) or not (or how much).
> 
> As far as I understand libev is pretty fine with fork, it simply listening
> for new events we send it via ev_feed_event explicitly. Of course libev
> also able to watch for SIGCHLD which we use as well. But to be honest
> at moment I dont see any problems with libev as itself. I think we should
> give all this a fly and wait for results.
> 
> > 
> > [1]: http://pod.tst.eu/http://cvs.schmorp.de/libev/ev.pod

In short: skip it.

Since there are no obvious problems, I would consider that everything
should be okay. It seems libev's comments would be applicable if we
would want to use libev in a child, but we don't.

> > >  - popen_write_timeout
> > > 	to write data into child's stdin with
> > > 	timeout
> > >  - popen_read_timeout
> > > 	to read data from child's stdout/stderr
> > > 	with timeout
> > 
> > My initial thought (see [2]) was that the popen engine will just give
> > several file descriptors, but coio_create() / coio_read_timeout() /
> > coio_write_timeout() / coio_close() will be called from a module that
> > implements Lua API for read / write streams.
> 
> Currently we use coio_read_timeout, coio_write_timeout helpers for IO.
> And out Lua API will be simply using popen_write_timeout and popen_read_timeout,
> without knowning which exactly transport we use inside popen engine.
> And this is critical -- top level API must know nothing about lowlevel
> implementation.
> 
> > 
> > This approach draws a solid line between process management and IO
> > management and would simplify them both. Are there problems with this
> > way?
> > 
> > [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013040.html

In short: let's postpone it.

I'll try to explain my idea. Popen API is in a sketch:

* popen_{init,free}                  -- (de)init the subsystem
* popen_{new,delete}                 -- spawn a child / delete a handle
* popen_{stat,command,state}         -- get info about a child
* popen_send_signal                  -- send a signal
* popen_{read_timeout,write_timeout} -- feed data from / to child's
                                        stdin / stdout / stderr

Let's look at this from Lua side:

* popen.new(<...>) -> handle
* handle:{command,stat}()
* handle:signal(<...>)
* handle.stdin:write(<...>)
* handle.stdout:read(<...>)
* handle.stderr:read(<...>)

It seems that stdin/stdout/stderr streams is a kind of separate module:
it manages coio watchers, store readahead buffers (say, to feed data
line by line to a user).

This brings the question into my head: should not popen backend just
expose file descriptors, which will be handled by another module?

I don't sure and it seems that the only way to understand what is better
is to try (at least for me).

The complexity that I expect is how to correctly link popen and io
stream objects, because in this implementation they would have different
lifetimes.

The gain I see is that this way we can implement io streams module that
is abstracted out from popen (in fact there is nothing popen specific in
pipes).

Anyway, let's start w/o this and look again when we'll build Lua API
upward this backend engine.

> > 
> > > +/**
> > > + * Handle SIGCHLD when a child process exit.
> > > + */
> > > +static void
> > > +popen_sigchld_handler(EV_P_ ev_child *w, int revents)
> > 
> > Are we really need to use those libev macros within our code? Our code
> > usually do:
> > 
> >  | ev_loop *loop = loop();
> >  | ev_<something>(loop, <...>);
> 
> This is libev api requirement.

In short: still actual.

I would not consider it as the API requirement. Let's grep our sources:
we actually pass the loop argument explicitly and it is much more
readable way.

> 
> > > +/**
> > > + * popen_send_signal - send a signal to a child process
> > > + * @handle:	popen handle
> > > + * @signo:	signal number
> > > + *
> > > + * Returns 0 on success, -1 otherwise.
> > > + */
> > > +int
> > > +popen_send_signal(struct popen_handle *handle, int signo)
> > > +{
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * A child may be killed or exited already.
> > > +	 */
> > > +	if (!popen_may_pidop(handle))
> > > +		return -1;
> > > +
> > > +	say_debug("popen: kill %d signo %d", handle->pid, signo);
> > > +	ret = kill(handle->pid, signo);
> > > +	if (ret < 0) {
> > > +		diag_set(SystemError, "Unable to kill %d signo %d",
> > > +			 handle->pid, signo);
> > > +	}
> > > +	return ret;
> > > +}
> > 
> > In some of previous versions of the patchset I saw unconditional
> > killpg() here. The ability to do it is often requested together with
> > setsid() in context of Python's subprocess.Popen(). Looks as important
> > feature, especially when a shell script is executed.
> > 
> > I think this should be configurable at least from the backend engine
> > perspective.
> 
> Well, you know currently we don't have the complete control over children
> process (a child can generate own children with own sessions and own
> groups) so usage of killpg doesn't give much profit to us. Plain kill
> is enough.

In short: still actual.

It depends. When running a shell pipeline it looks useful to do setsid +
killpg. And, again, as I see (by SO questions) it is often asked in
context of Python's popen.

I would add a flag for this purpose.

> 
> > > +	/*
> > > +	 * A caller must preserve space for this.
> > > +	 */
> > > +	if (opts->flags & POPEN_FLAG_SHELL) {
> > > +		opts->argv[0] = "sh";
> > > +		opts->argv[1] = "-c";
> > > +	}
> > 
> > I would let a caller do this. The code of the backend engine tends to be
> > general and whether to add 'sh -c' and whether it should assume setsid()
> > + killpg() looks more as calling code matter.
> 
> No, current examples of API (in python for example) doesn't require
> callers to provide "sh", "-c". This mark about a space is rather
> for future code which will be providing Lua api.

In short: still actual.

Now the contract between a caller (say, Lua module) and the popen
backend looks so: a caller should left a space for "sh" and "-c", add
the corresponding flag, but don't add "sh" and "-c" itself. I think that
there is no reason to make it so complex. We can remove the flag and
state that the popen engine just run processes disregarding whether it
is a system shell, other interpreter or any other process. We can just
remove the flag and the API will look simpler w/o loss any ability. The
contract will become more clean: we'll not hide details how exactly
POPEN_FLAG_SHELL is handled (in fact it does almost nothing).

I think it is more natural to let a caller (a Lua wrapper for popen) to
add "sh", "-c", set POPEN_FLAG_SETSID | POPEN_FLAG_PGKILL and call it
'shell mode' or so.

WBR, Alexander Turenko.