From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 7 May 2019 11:49:49 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH] core: Non-blocking io.popen Message-ID: <20190507084949.kgjnkxflpeofpgnd@esperanza> References: <20190424170032.10726-1-szudin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190424170032.10726-1-szudin@tarantool.org> To: Stanislav Zudin Cc: tarantool-patches@freelists.org, Alexander Turenko List-ID: [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: 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.