From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BF5104696C3 for ; Mon, 20 Apr 2020 04:28:59 +0300 (MSK) Date: Mon, 20 Apr 2020 04:21:52 +0300 From: Igor Munkin Message-ID: <20200420012152.GW8314@tarantool.org> References: <20200419123845.GP8314@tarantool.org> <20200419222434.nnus7jnm2hgmeq6p@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200419222434.nnus7jnm2hgmeq6p@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH 2/2] popen: add popen Lua module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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. > > ---- > > > > > @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. > > > > > 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! > > > > > 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” :) > -- Best regards, IM