From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 67E8D469719 for ; Tue, 3 Mar 2020 13:41:48 +0300 (MSK) Date: Tue, 3 Mar 2020 13:41:44 +0300 From: Alexander Turenko Message-ID: <20200303104144.fli54ssn25k7ftxt@tkn_work_nb> References: <20200131192504.12142-1-gorcunov@gmail.com> <20200131192504.12142-3-gorcunov@gmail.com> <20200216230442.2rk4az4ow6gyjkjl@tkn_work_nb> <20200217091147.GH2527@uranus> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200217091147.GH2527@uranus> Subject: Re: [Tarantool-patches] [PATCH v10 2/3] popen: introduce a backend engine List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml 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_(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.