From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 959624696C3 for ; Sat, 18 Apr 2020 07:14:03 +0300 (MSK) From: Alexander Turenko Date: Sat, 18 Apr 2020 07:13:54 +0300 Message-Id: <814cb0a3d1a14158c3e378624b9fb1d122cd53f8.1587172237.git.alexander.turenko@tarantool.org> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH 1/2] popen: always free resources in popen_delete() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , Igor Munkin Cc: tarantool-patches@dev.tarantool.org The function still set a diagnostics when a signal sending fails and returns -1, but it is purely informational result: for logging or so. It reflects notes about dealing with failures in Linux's `man 2 close`: | Note, however, that a failure return should be used only for | diagnostic purposes <...> or remedial purposes <...>. | | <...> Linux kernel always releases the file descriptor early in the | close operation, freeing it for reuse; the steps that may return an | error <...> occur only later in the close operation. | | Many other implementations similarly always close the file descriptor | <...> even if they subsequently report an error on return from | close(). POSIX.1 is currently silent on this point, but there are | plans to mandate this behavior in the next major release of the | standard. When kill or killpg returns EPERM a caller usually unable to overcome it somehow: retrying is not sufficient here. So there are no needs to keep the handle: a caller refuses the handle and don't want to perform any other operation on it. The open engine do its best to kill a child process or a process group, but when it is not possible, just set the a diagnostic and free handle resources anyway. Left comments about observed Mac OS behaviour regarding killing a process group, where all processes are zombies (or just when a process group leader is zombie, don't sure): it gives EPERM instead of ESRCH from killpg(). This result should not surprise a user, so it should be documented. See [1] for another description of the problem (I don't find any official information about this). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1329528 Part of #4031 --- src/lib/core/popen.c | 88 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 15 deletions(-) diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c index 3a12a439b..da4b89757 100644 --- a/src/lib/core/popen.c +++ b/src/lib/core/popen.c @@ -688,11 +688,27 @@ popen_state_str(unsigned int state) * Possible errors: * * - SystemError: a process does not exists anymore + * + * Aside of a non-exist process it is also + * set for a zombie process or when all + * processes in a group are zombies (but + * see note re Mac OS below). + * * - SystemError: invalid signal number + * * - SystemError: no permission to send a signal to * a process or a process group * - * Set errno to ESRCH when a process does not exist. + * It is set on Mac OS when a signal is sent + * to a process group, where a group leader + * is zombie (or when all processes in it + * are zombies, don't sure). + * + * Whether it may appear due to other + * reasons is unclear. + * + * Set errno to ESRCH when a process does not exist or is + * zombie. */ int popen_send_signal(struct popen_handle *handle, int signo) @@ -732,39 +748,74 @@ popen_send_signal(struct popen_handle *handle, int signo) /** * Delete a popen handle. * - * The function performs the following actions: + * Send SIGKILL and free the handle. * - * - Kill a child process or a process group depending of - * POPEN_FLAG_GROUP_SIGNAL (if the process is alive and - * POPEN_FLAG_KEEP_CHILD flag is not set). - * - Close all fds occupied by the handle. - * - Remove the handle from a living list. - * - Free all occupied memory. + * Details about signaling: * - * @see popen_send_signal() for note about ..._GROUP_SIGNAL. + * - The signal is sent only when ...KEEP_CHILD is not set. + * - The signal is sent only when a process is alive according + * to the information available on current even loop iteration. + * (There is a gap here: a zombie may be signaled; it is + * harmless.) + * - The signal is sent to a process or a grocess group depending + * of ..._GROUP_SIGNAL flag. @see popen_send_signal() for note + * about ..._GROUP_SIGNAL. * - * Return 0 at success and -1 at failure (and set a diag). + * Resources are released disregarding of whether a signal + * sending succeeds: all fds occupied by the handle are closed, + * the handle is removed from a living list, all occupied memory + * is freed. * - * Possible errors: + * The function may return 0 or -1 (and set a diag) as usual, + * but it always frees the handle resources. So any return + * value usually means success for a caller. The return + * value and diagnostics are purely informational: it is + * for logging or same kind of reporting. + * + * Possible diagnostics (don't consider them as errors): * * - SystemError: no permission to send a signal to * a process or a process group + * + * This error may appear due to Mac OS + * behaviour on zombies when + * ..._GROUP_SIGNAL is set, + * @see popen_send_signal(). + * + * Whether it may appear due to other + * reasons is unclear. + * + * Always return 0 when a process is known as dead: no signal + * will be send, so no 'failure' may appear. */ int popen_delete(struct popen_handle *handle) { + /* + * Save a result and a failure reason of the + * popen_send_signal() call. + */ + int rc = 0; + struct diag *diag = diag_get(); + struct error *e = NULL; + size_t i; assert(handle != NULL); if ((handle->flags & POPEN_FLAG_KEEP_CHILD) == 0) { /* - * Unable to kill the process -- give an error. + * Unable to kill the process -- save the error + * and pass over. * The process is not exist already -- pass over. */ if (popen_send_signal(handle, SIGKILL) != 0 && - errno != ESRCH) - return -1; + errno != ESRCH) { + rc = -1; + e = diag_last_error(diag); + assert(e != NULL); + error_ref(e); + } } for (i = 0; i < lengthof(handle->ios); i++) { @@ -787,7 +838,14 @@ popen_delete(struct popen_handle *handle) rlist_del(&handle->list); handle_free(handle); - return 0; + + /* Restore an error from popen_send_signal() if any. */ + if (rc != 0) { + diag_set_error(diag, e); + error_unref(e); + } + + return rc; } /** -- 2.25.0