From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149])
 (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 4AB2C46970E
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 20 Dec 2019 15:11:56 +0300 (MSK)
Date: Fri, 20 Dec 2019 15:11:52 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20191220121152.mts76z3frywkej2e@tkn_work_nb>
References: <20191217125420.20881-1-gorcunov@gmail.com>
 <20191217125420.20881-3-gorcunov@gmail.com>
 <20191220081159.GG30445@atlas>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20191220081159.GG30445@atlas>
Subject: Re: [Tarantool-patches] [PATCH v6 2/4] popen: Introduce a backend
	engine
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Konstantin Osipov <kostja.osipov@gmail.com>, Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org>, Kirill Yukhin <kyukhin@tarantool.org>, Igor Munkin <imun@tarantool.org>

> > +	if (handle->flags & POPEN_FLAGS_SETSID) {
> > +		if (signo == SIGKILL || signo == SIGTERM) {
> > +			say_debug("popen: killpg %d signo %d",
> > +				  handle->pid, signo);
> > +			ret = killpg(handle->pid, signo);
> 
> I don't understand why you need to use killpg() here.
> 
> Could you please explain?
> I suspect that you don't, you're just trying to be safe,
> and there is no need to.

I guess it is quite common to call setsid() in a child to then able to
kill a process group at whole, based on the note:

> Note: If you need to modify the environment for the child use the env
> parameter rather than doing it in a preexec_fn. The start_new_session
> parameter can take the place of a previously common use of preexec_fn
> to call os.setsid() in the child. 

https://docs.python.org/3.9/library/subprocess.html

It is quite useful when a shell is called: a non-interactive shell does
not perform job control and so does not create a separate process group
for a command pipeline.

I googled a bit to verify that os.setsid() is actually often suggested
with subprocess.Popen() and, yep, this is so:
https://stackoverflow.com/a/4791612/1598057

I think that it is okay to be a default behaviour (because it is
harmless even for a singleton command). I guess that Python's API does
not enable it default for historical reasons.

OTOH, this behaviour means that we need to kill all spawned groups at
tarantool exit, because they will not be killed with, say, SIGINT from a
terminal (I didn't verify whether the current implementation does it).

WBR, Alexander Turenko.