From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f67.google.com (mail-lf1-f67.google.com [209.85.167.67]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 06B41469719 for ; Mon, 17 Feb 2020 12:11:50 +0300 (MSK) Received: by mail-lf1-f67.google.com with SMTP id z26so11254922lfg.13 for ; Mon, 17 Feb 2020 01:11:50 -0800 (PST) Date: Mon, 17 Feb 2020 12:11:47 +0300 From: Cyrill Gorcunov Message-ID: <20200217091147.GH2527@uranus> References: <20200131192504.12142-1-gorcunov@gmail.com> <20200131192504.12142-3-gorcunov@gmail.com> <20200216230442.2rk4az4ow6gyjkjl@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200216230442.2rk4az4ow6gyjkjl@tkn_work_nb> 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: Alexander Turenko Cc: tml 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_(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 :)