Re: [tarantool-patches] [PATCH] core: Non-blocking io.popen

[Vladimir Davydov <vdavydov.dev@gmail.com>]

[Cc += Georgy re popen API]

On Tue, May 07, 2019 at 11:49:49AM +0300, Vladimir Davydov wrote:
> [Cc += Alexander re popen API]
> 
> On Wed, Apr 24, 2019 at 08:00:32PM +0300, Stanislav Zudin wrote:
> > Adds nonblocking implementation of popen.
> > The method is available in namespace fio.
> > fio.popen() returns an object with three methods:
> > read(), write() and close().
> > Method read() reads both STDOUT and STDERR output.
> > 
> > Closes #4031
> 
> Please write a DocBot request and be more thorough when describing the
> new API: it should be clear what all the functions do and what arguments
> they expect; there should be some examples. Currently it's unclear how
> to use the new module judging by the description.
> 
> Anyway, the API doesn't look good enough to me:

We've discussed the proposed API with Georgy and Alexander. Georgy has
convinced us that it'd be best if read() returned the output of both
stdout/stderr with the second value indicating where the output came
from, because this is the most flexible variant: one can read both
streams together or separately thanks to multireturn. So points 1 and 3
below aren't valid

However, we still want to redirect any of stdin/stderr/stdout to
/dev/null or any arbitrary file descriptor, forcefully terminate the
process, and wait for it to exit.  We see it now that popen() should
return a special kind of object, which has read(), write(), close(),
kill(), and wait() methods.  I guess all blocking methods should have
optional timeouts. Probably, we need to be able to close stdin, stdout,
stderr separately (close should have an argument?), and wait() should
return the exit code on success. Also, it'd be great if one could
redirect the output of one popen to another popen, like demonstrated
here:

  https://www.python.org/dev/peps/pep-0324/#replacing-shell-pipe-line

Please prepare a mock-up of the API taking into account these
comments and send it for review to server-dev@: we need to agree on it
with the Solutions team.

I only skimmed through the code, haven't looked through it thoroughly
yet, and so only have a few minor comments:

 - Please don't use -2 as return code. Looks ugly. Use errno or pass
   this info in an extra argument.
 - I'm not quite sure it's worth moving fork() out of the tx thread.
   So let's factor it out into a separate patch: first implement popen
   with fork in tx, then move fork out of tx. Also, please consider
   getting rid of atfork handlers - they look legacy.
 - We need to close all file descriptors in a child process after fork.
   Probably, we should use O_CLOSEXEC for all file descriptors. Please
   do it in a separate patch.

> 
>  1. Reading both stdout and stderr with the same method doesn't make any
>     sense. Those are two separate streams; each of them should have a
>     separate file handle.
>  2. There must be a way to ignore any of stdin/stdout/stderr. E.g. the
>     caller might not be interested in stdout. However, if he doesn't
>     read it, the program will just block once the pipe buffer has been
>     filled. This is very inconvenient. I guess, we should use parent's
>     stdout/stderr by default, but there also must be a way to completely
>     silence the child's output (/dev/null).
>  3. Is it really necessary to introduce separate read/write methods for
>     popen? Can't we reuse existing fio.read/write? After all, a pipe can
>     be used (almost) just like a normal file so why not simply wrap it
>     in fio file handle?
>  4. There must be a way to forcefully terminate a child program (kill).
>  5. The caller should be able to obtain the exit code of a terminated
>     child - without it it'd be impossible to figure out whether the
>     program succeeded or failed.
> 
> I guess we have to go back to the drawing board and try to devise a good
> API before proceeding to implementation. It may be worth to take a look
> at other languages' versions of popen, e.g. Python's. Please try to come
> up a good API and send out an RFC so that others can look at it and
> comment.
> 
> Also, the test is insufficient: you should test all the functions
> you introduce, all possible use cases. E.g. you don't check that
> write(stderr) or read(stdin) works AFAICS.