Re: [Tarantool-patches] [PATCH 10/12] popen: add FIXME re group signal flaw

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Tue, Apr 14, 2020 at 04:19:10PM +0300, Cyrill Gorcunov wrote:
> On Tue, Apr 14, 2020 at 02:38:19PM +0300, Alexander Turenko wrote:
> > It is convenient to have such anchors for known problems.
> > 
> > Part of #4031
> Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> Actually I need to think about all this more. But I think
> it is safe to merge now.

I think about it more and also briefly discussed with Cyrill.

It seems this problem is a kind of fundamental and we should explain it
in the documentation comments rather than mark to fix later.

So I rewrote the commit in this way:

commit 234b184ffdd9dce234520643f12fec12bbc9fa1a
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Mon Apr 13 13:37:55 2020 +0300

    popen: clarify group signaling details
    
    Even when ..._SETSID and ..._GROUP_SIGNAL are set, we unable to safely
    kill a process group after the child process we spawned becomes died. So
    we don't do that.
    
    The behaviour seems to be indefeasible part of Unix process group
    design. The best that we can do here is describe those details in the
    documentation comment.
    
    NB: It seems that pid namespaces allow to overcome this problem, however
    it is the Linux specific feature, so we unlikely will use them.
    
    Part of #4031

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 411aad03b..5cd7926d1 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -539,6 +539,13 @@ popen_state_str(unsigned int state)
  * When POPEN_FLAG_GROUP_SIGNAL is set the function sends
  * a signal to a process group rather than a process.
  *
+ * A signal will not be sent if the child process is already
+ * dead: otherwise we might kill another process that occupies
+ * the same PID later. This means that if the child process
+ * dies before its own childs, the function will not send a
+ * signal to the process group even when ..._SETSID and
+ * ..._GROUP_SIGNAL are set.
+ *
  * Return 0 at success or -1 at failure (and set a diag).
  *
  * Possible errors:
@@ -597,6 +604,8 @@ popen_send_signal(struct popen_handle *handle, int signo)
  * - Remove the handle from a living list.
  * - Free all occupied memory.
  *
+ * @see popen_send_signal() for note about ..._GROUP_SIGNAL.
+ *
  * Return 0 at success and -1 at failure (and set a diag).
  *
  * Possible errors:
diff --git a/src/lib/core/popen.h b/src/lib/core/popen.h
index 4cdd95175..8cb71e28d 100644
--- a/src/lib/core/popen.h
+++ b/src/lib/core/popen.h
@@ -99,6 +99,8 @@ enum popen_flag_bits {
 
 	/*
 	 * Send signal to a process group.
+	 *
+	 * @see popen_send_signal() for details.
 	 */
 	POPEN_FLAG_GROUP_SIGNAL_BIT	= 16,
 	POPEN_FLAG_GROUP_SIGNAL		= (1 << POPEN_FLAG_GROUP_SIGNAL_BIT),