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

[Cyrill Gorcunov <gorcunov@gmail.com>]

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
> 
> >  - 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
> 
> > +/**
> > + * 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.

> > +/**
> > + * 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.

> > +	/*
> > +	 * 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.

Anyway, I'm about to send new version today since testing revealed
some nits in this code. Thus, stay tuned :)