From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Fri, 20 Dec 2019 15:11:56 +0300 (MSK) Date: Fri, 20 Dec 2019 15:11:52 +0300 From: Alexander Turenko 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , Cyrill Gorcunov , tml , Kirill Yukhin , Igor Munkin > > + 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.