Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thanks for the careful proofreading!

Aside of changes below I changed `popen.stream.OPENED (== 'opened')` to
`popen.stream.OPEN (== 'open')` to match english grammar. Same for
statuses -> status (yep, 'statuses' is allowed in English, but it is
ugly).

Force-pushed Totktonada/gh-4031-popen-13-full-ci.

WBR, Alexander Turenko.

----

> > The `popen` module provides two functions to create that named popen
> > object `popen.shell` which is the similar to libc `popen` syscall and
> 
> Typo: I guess a colon is missing here.

Thanks! Fixed.

> > Example:
> > 
> >  | local popen = require('popen')
> >  |
> >  | local ph = popen.shell('date', 'r')
> >  | local date = ph:read():rstrip()
> 
> Minor: IMHO <rstrip> call might lead to a misuse, considering there is
> no word about it in description below. I guess you can drop it.

It is quite minor IMHO, but I don't mind.

Added the explanation to .shell() function description in the code and
the commit message:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 40f4343eb..8ae01b3f1 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1408,6 +1396,18 @@ err:
  *
  * Execute 'sh -c date' command, read the output and close the
  * popen object.
+ *
+ * Unix defines a text file as a sequence of lines, each ends
+ * with the newline symbol. The same convention is usually
+ * applied for a text output of a command (so when it is
+ * redirected to a file, the file will be correct).
+ *
+ * However internally an application usually operates on
+ * strings, which are NOT newline terminated (e.g. literals
+ * for error messages). The newline is usually added right
+ * before a string is written to the outside world (stdout,
+ * console or log). :rstrip() in the example above is shown
+ * for this sake.
  */
 static int
 lbox_popen_shell(struct lua_State *L)

I left .new() description intact: it should be described once and
.shell() going first in the documentation request (commit message).

> > Execute a child program in a new process.
> > 
> > @param argv  an array of a program to run with
> >              command line options, mandatory;
> >              absolute path to the program is required
> 
> Absolute path is not required when shell option is set to <true>.

Good catch!

Added to the comment in the code and to the commit message:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 2aa0a5851..bf06bbfa7 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1114,6 +1114,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  * @param argv  an array of a program to run with
  *              command line options, mandatory;
  *              absolute path to the program is required
+ *              when @a opts.shell is false (default)
  *
  * @param opts  table of options
  *

> > @param opts.env  an array of environment variables to
> >                  be used inside a process;
> >                  - when is not set then the current
> >                    environment is inherited;
> >                  - if set then the environment will be
> >                    replaced
> >                  - if set to an empty array then the
> >                    environment will be dropped
> 
> Minor: I looks more convenient to sort the lines above the following
> way:
> * when is not set <...>
> * if set to an empty table <...>
> * if set to a non-empty table <...>

I don't mind.

Reordered in the code and the commit message.

Also I rephrased the description a bit to highlight the fact that the
parameter is key-value mapping, not an array.

diff --git a/src/lua/popen.c b/src/lua/popen.c
index bf06bbfa7..f17e3faf1 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1134,14 +1134,16 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *                         feed data from/to the fd to parent
  *                         using a pipe
  *
- * @param opts.env  an array of environment variables to
- *                  be used inside a process;
+ * @param opts.env  a table of environment variables to
+ *                  be used inside a process; key is a
+ *                  variable name, value is a variable
+ *                  value.
  *                  - when is not set then the current
  *                    environment is inherited;
+ *                  - if set to an empty table then the
+ *                    environment will be dropped
  *                  - if set then the environment will be
  *                    replaced
- *                  - if set to an empty array then the
- *                    environment will be dropped
  *
  * @param opts.shell            (boolean, default: false)
  *        true                  run a child process via

> > @param opts.setsid           (boolean, default: false)
> >        true                  start executable inside a new
> >                              session
> >        false                 don't do that
> 
> Minor: I reword it in more clear way below:
> | @param opts.setsid           (boolean, default: false)
> |        true                  run the program in a new
> |                              session
> |        false                 run the program in the
> |                              tarantool instance's
> |                              session

So I would add "...and process group". Changed:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index f17e3faf1..ec8c95662 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1151,9 +1151,11 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *        false                 call the executable directly
  *
  * @param opts.setsid           (boolean, default: false)
- *        true                  start executable inside a new
+ *        true                  run the program in a new
  *                              session
- *        false                 don't do that
+ *        false                 run the program in the
+ *                              tarantool instance's
+ *                              session and process group
  *
  * @param opts.close_fds        (boolean, default: true)
  *        true                  close all inherited fds from a

> > The returned handle is subject to garbage collection, but
> > it may be freed explicitly using :close(). SIGKILL is sent
> > to the child process at :close() / GC if it is alive and
> > opts.keep_child is not set.
> 
> Minor: Let's adjust the terminology. What do you mean speaking about
> "handle"? If it's a GC object, then there is no way to release it
> manually or "explicitly", only via platform GC. If it's a GC object
> *payload* then it's not managed by Lua GC and need to be explicitly
> released via special method or the corresponding __gc metamethod.

My intention is to give two ideas here:

* Feel free to use explicit :close() or lean on Lua GC.
* :close() / GC handler kills the child process.

But being strict in terms and wording is goal for me too, so I reword
the paragraph to eliminate phrases, which requires a GC object / a GC
object payload as an object / a subject.

Changed in the code and the commit:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index ec8c95662..31b4bc2b0 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1184,10 +1184,11 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *                              :close() or collecting of the
  *                              handle by Lua GC
  *
- * The returned handle is subject to garbage collection, but
- * it may be freed explicitly using :close(). SIGKILL is sent
- * to the child process at :close() / GC if it is alive and
- * opts.keep_child is not set.
+ * The returned handle provides :close() method to explicitly
+ * release all occupied resources (including the child process
+ * itself if @a opts.keep_child is not set). However if the
+ * method is not called for a handle during its lifetime, the
+ * same freeing actions will be triggered by Lua GC.
  *
  * It is recommended to use opts.setsid + opts.group_signal
  * if a child process may spawn its own childs and they all

> > Example 1:
> > 
> >  | local popen = require('popen')
> >  |
> >  | local ph = popen.new({'/bin/date'}, {
> >  |     stdout = popen.opts.PIPE,
> >  | })
> >  | local date = ph:read():rstrip()
> >  | ph:close()
> >  | print(date) -- Thu 16 Apr 2020 01:40:56 AM MSK
> > 
> > Execute 'date' command, read the result and close the
> > popen object.
> 
> Minor: Again, no mention about rstrip.

I think once is enough. Added the popen.shell() description.

> > Example 4:
> > 
> >  | local function call_jq(stdin, filter)
> 
> Minor: <stdin> look a bit misleading. What about <input>, <json>, etc?

Okay, 'input'.

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 31b4bc2b0..bf79a5d29 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1270,7 +1270,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *
  * Example 4:
  *
- *  | local function call_jq(stdin, filter)
+ *  | local function call_jq(input, filter)
  *  |     -- Start jq process, connect to stdin, stdout and stderr.
  *  |     local jq_argv = {'/usr/bin/jq', '-M', '--unbuffered', filter}
  *  |     local ph, err = popen.new(jq_argv, {
@@ -1281,7 +1281,7 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  *  |     if ph == nil then return nil, err end
  *  |
  *  |     -- Write input data to child's stdin and send EOF.
- *  |     local ok, err = ph:write(stdin)
+ *  |     local ok, err = ph:write(input)
  *  |     if not ok then return nil, err end
  *  |     ph:shutdown({stdin = true})
  *  |

> >  |     -- Read diagnostics from stderr if any.
> >  |     local err = ph:read({stderr = true})
> >  |     if err ~= '' then
> >  |         ph:close()
> >  |         return nil, err
> >  |     end
> >  |
> >  |     -- Glue all chunks, strip trailing newline.
> 
> Side note: This comment is definitely needed near <rstrip> calls above.

Added once, to popen.shell() example:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index bf79a5d29..6cfb268a1 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1395,9 +1395,16 @@ err:
  *
  *  | local popen = require('popen')
  *  |
+ *  | -- Run the program and save its handle.
  *  | local ph = popen.shell('date', 'r')
+ *  |
+ *  | -- Read program's output, strip trailing newline.
  *  | local date = ph:read():rstrip()
+ *  |
+ *  | -- Free resources. The process is killed (but 'date'
+ *  | -- exits itself anyway).
  *  | ph:close()
+ *  |
  *  | print(date)
  *
  * Execute 'sh -c date' command, read the output and close the

> > If a process writes large text to stderr, it may fill out
> > stderr pipe buffer and block in write(2, ...). So we need to
> > read stderr too in another fiber to handle this case.
> 
> Typo: s/stderr too in another fiber/stderr in another fiber too/.

I want to avoid possible understanding as 'read stderr here and in
another fiber too'. I can just remove 'too'. I also changed 'block' to
'stuck':

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 6cfb268a1..6835286c7 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1318,8 +1318,8 @@ luaT_popen_parse_mode(struct lua_State *L, int idx)
  * another fiber (start it before a first :write()).
  *
  * If a process writes large text to stderr, it may fill out
- * stderr pipe buffer and block in write(2, ...). So we need to
- * read stderr too in another fiber to handle this case.
+ * stderr pipe buffer and stuck in write(2, ...). So we need
+ * to read stderr in a separate fiber to handle this case.
  */
 static int
 lbox_popen_new(struct lua_State *L)

> > Read data from stdout or stderr streams with @a timeout.
> > By default it reads from stdout. Set @a opts.stderr to
> > `true` to read from stderr.
> 
> If only <stderr> is set to true, is stdout output also captured?

Nope. There is an error about it in the list below, but, yep, it worth
to highlight it. Added the note:

diff --git a/src/lua/popen.c b/src/lua/popen.c
index 6835286c7..a071065b4 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -1647,6 +1647,9 @@ lbox_popen_wait(struct lua_State *L)
  * By default it reads from stdout. Set @a opts.stderr to
  * `true` to read from stderr.
  *
+ * It is not possible to read from stdout and stderr both in
+ * one call. Set either @a opts.stdout or @a opts.stderr.
+ *
  * Raise an error on incorrect parameters or when the fiber is
  * cancelled:
  *

> > Return `nil, err` on a failure. Possible reasons:
> > 
> > - SocketError: an IO error occurs at read().
> 
> SocketError on pipe looks ridiculous to me. Maybe an IO one?

It comes from coio.cc. It would be good to refactor it someday, but not
tonight :)

> > `popen_handle:shutdown(opts) -> true`
> 
> Minor: Considering the method description, <eof> looks more convenient
> name here. On the other hand <eof> name might be misguiding one
> considering feof(3) standart C function.

:close() for ph.{stdin,stdout,stderr} would be ideal, but we don't have
something like separate read / write streams now. We have bidirectional
handle for reading and writing, so the nearest counterpart would be
socket's shutdown().

More on this: closing of stdin is EOF for a child, but closing of stdout
/ stderr will lead to SIGPIPE (+EPIPE in errno) at write() in the child.

I think the name we have now is good.

> > `popen_handle:info() -> res, err`
> 
> <err> is never returned.

Good catch!

Fixed.

> > `popen_handle:wait() -> res, err`
> 
> <err> is never returned.

Thanks!

Fixed.