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

[Igor Munkin <imun@tarantool.org>]

Sasha,

On 20.04.20, Alexander Turenko wrote:
> 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).

Yep, that's the way much better.

> 
> Force-pushed Totktonada/gh-4031-popen-13-full-ci.
> 
> WBR, Alexander Turenko.
> 
> ----
> 

<snipped>

> 
> > > @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.

Good remark, thanks.

> 

<snipped>

> 
> > > 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

Nice, now it's clear, thanks!

> 

<snipped>

> 
> > > 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 :)

And there is only one thing we say to Refactoring: “not today” :)

> 

<snipped>

-- 
Best regards,
IM