Re: [Tarantool-patches] [PATCH v6 2/4] popen: Introduce a backend engine

[Alexander Turenko <alexander.turenko@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.