daemon: explicitly allow EINTR during poll() #2002
Open
+11
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5af3b11 to
a450bdb
Compare
|
/submit |
|
Submitted as pull.2002.git.git.1750774122.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this), regarding a450bdb (outdated): This will need the following fixup on top:
---- >8 ----
diff --git a/daemon.c b/daemon.c
index 542e638223..f5f0c426c3 100644
--- a/daemon.c
+++ b/daemon.c
@@ -912,7 +912,7 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
add_child(&cld, addr, addrlen);
}
-static void child_handler(int signo)
+static void child_handler(int signo UNUSED)
{
/*
* Empty handler because systemcalls should get interrupted |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this), regarding a450bdb (outdated): "Carlo Marcelo Arenas Belón via GitGitGadget"
<gitgitgadget@gmail.com> writes:
> -static void child_handler(int signo UNUSED)
> +static void child_handler(int signo)
> {
> /*
> - * Otherwise empty handler because systemcalls will get interrupted
> - * upon signal receipt
> + * Empty handler because systemcalls should get interrupted
> + * upon signal receipt.
> */
> +#ifdef NO_SIGINTERRUPT
> + /* SysV needs the handler to be rearmed */
> + signal(signo, child_handler);
> +#endif
> }
On NO_SIGINTERRUPT systems, signo is UNUSED, isn't it?
Can we abstract this a bit better? #if/#else/#endif sprinkled
everywhere is simply an eyesore.
> static int set_reuse_addr(int sockfd)
> @@ -1118,8 +1122,10 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>
> static int service_loop(struct socketlist *socklist)
> {
> - struct pollfd *pfd;
> +#ifndef NO_SIGINTERRUPT
> struct sigaction sa;
> +#endif
> + struct pollfd *pfd;
> CALLOC_ARRAY(pfd, socklist->nr);
>
> @@ -1128,14 +1134,22 @@ static int service_loop(struct socketlist *socklist)
> pfd[i].events = POLLIN;
> }
>
> +#ifdef NO_SIGINTERRUPT
> + signal(SIGCHLD, child_handler);
> +#else
> sigemptyset(&sa.sa_mask);
> sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> sa.sa_handler = child_handler;
> sigaction(SIGCHLD, &sa, NULL);
> +#endif
>
> for (;;) {
> check_dead_children();
>
> +#ifndef NO_SIGINTERRUPT
> + sa.sa_flags &= ~SA_RESTART;
> + sigaction(SIGCHLD, &sa, NULL);
> +#endif
> if (poll(pfd, socklist->nr, -1) < 0) {
> if (errno != EINTR) {
> logerror("Poll failed, resuming: %s",
> @@ -1144,6 +1158,10 @@ static int service_loop(struct socketlist *socklist)
> }
> continue;
> }
> +#ifndef NO_SIGINTERRUPT
> + sa.sa_flags |= SA_RESTART;
> + sigaction(SIGCHLD, &sa, NULL);
> +#endif |
|
This patch series was integrated into seen via 8fdef1e. |
|
User |
9d68ce8 to
b737e03
Compare
|
/submit |
|
Submitted as pull.2002.v2.git.git.1750836928.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> This series addresses and ambiguity that is at least visible in OpenBSD,
> where zombie proceses would only be cleared after a new connection is
> received.
There is still a race where a child that exits after it has been checked in check_dead_children() but before we call poll() will not be collected until a new connection is received or a child exits while we're polling. If we used the self-pipe trick described on the select(2) man page [1] we would avoid that race and would not need to mess with SA_RESTART and so would not need to introduce USE_NON_POSIX_SIGNAL.
Best Wishes
Phillip
[1] https://www.man7.org/linux/man-pages/man2/select.2.html
> The underlying problem is that when this code was originally introduced,
> SA_RESTART was not widely implemented, and the signal() call usually
> implemented SysV like semantics, at least until it started being
> reimplemented by calling sigaction() internally.
> > Changes since v1
> > * Almost all references to siginterrupt has been removed and a better named
> variable used instead
> * Changes had been anstracted to minimize ifdefs and their introduction
> staged more naturally
> > Carlo Marcelo Arenas Belón (3):
> compat/posix.h: track SA_RESTART fallback
> daemon: use sigaction() to install child_handler()
> daemon: explicitly allow EINTR during poll()
> > Makefile | 6 +++++
> compat/mingw-posix.h | 1 -
> compat/posix.h | 8 +++++++
> config.mak.uname | 7 +++---
> configure.ac | 17 +++++++++++++++
> daemon.c | 52 +++++++++++++++++++++++++++++++++++++++-----
> meson.build | 4 ++++
> 7 files changed, 85 insertions(+), 10 deletions(-)
> > > base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v2
> Pull-Request: https://github.com/git/git/pull/2002
> > Range-diff vs v1:
> > 1: 2b5a58e53ac ! 1: e82b7425bbc compat/posix.h: track SA_RESTART fallback
> @@ Metadata
> ## Commit message ##
> compat/posix.h: track SA_RESTART fallback
> > - Systems without SA_RESTART where using custom CFLAGS instead of
> - the standard header file.
> + Systems without SA_RESTART are using custom CFLAGS or headers
> + instead of the standard header file.
> > - Consolidate that, so it will be easier to use in a future commit.
> + Correct that, and invent a Makefile variable to track the
> + exceptions which will become handy in the next commits.
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > + ## Makefile ##
> +@@ Makefile: include shared.mak
> + # when attempting to read from an fopen'ed directory (or even to fopen
> + # it at all).
> + #
> ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> ++# prefer to use ANSI C signal() over POSIX sigaction()
> ++#
> + # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR
> + # when a signal is received (as opposed to restarting).
> + #
> +@@ Makefile: ifdef FREAD_READS_DIRECTORIES
> + COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
> + COMPAT_OBJS += compat/fopen.o
> + endif
> ++ifdef USE_NON_POSIX_SIGNAL
> ++ COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
> ++endif
> + ifdef OPEN_RETURNS_EINTR
> + COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
> + endif
> +
> + ## compat/mingw-posix.h ##
> +@@ compat/mingw-posix.h: struct sigaction {
> + sig_handler_t sa_handler;
> + unsigned sa_flags;
> + };
> +-#define SA_RESTART 0
> +
> + struct itimerval {
> + struct timeval it_value, it_interval;
> +
> ## compat/posix.h ##
> @@ compat/posix.h: char *gitdirname(char *);
> #define NAME_MAX 255
> #endif
> > -+/* On most systems <signal.h> would have given us this, but
> ++/*
> ++ * On most systems <signal.h> would have given us this, but
> + * not on some systems (e.g. NonStop, QNX).
> + */
> +#ifndef SA_RESTART
> -+#define SA_RESTART 0 /* disabled for sigaction() */
> ++# define SA_RESTART 0 /* disabled for sigaction() */
> +#endif
> +
> typedef uintmax_t timestamp_t;
> @@ compat/posix.h: char *gitdirname(char *);
> #define parse_timestamp strtoumax
> > ## config.mak.uname ##
> +@@ config.mak.uname: ifeq ($(uname_S),Windows)
> + NO_STRTOUMAX = YesPlease
> + NO_MKDTEMP = YesPlease
> + NO_INTTYPES_H = YesPlease
> ++ USE_NON_POSIX_SIGNAL = YesPlease
> + CSPRNG_METHOD = rtlgenrandom
> + # VS2015 with UCRT claims that snprintf and friends are C99 compliant,
> + # so we don't need this:
> @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
> FREAD_READS_DIRECTORIES = UnfortunatelyYes
> > @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
> # Apparently needed in compat/fnmatch/fnmatch.c.
> COMPAT_CFLAGS += -DHAVE_STRING_H=1
> NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
> +@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
> + NO_MMAP = YesPlease
> + NO_POLL = YesPlease
> + NO_INTPTR_T = UnfortunatelyYes
> ++ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
> + CSPRNG_METHOD = openssl
> + SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
> + SHELL_PATH = /usr/coreutils/bin/bash
> +@@ config.mak.uname: ifeq ($(uname_S),MINGW)
> + NEEDS_LIBICONV = YesPlease
> + NO_STRTOUMAX = YesPlease
> + NO_MKDTEMP = YesPlease
> ++ USE_NON_POSIX_SIGNAL = YesPlease
> + NO_SVN_TESTS = YesPlease
> +
> + # The builtin FSMonitor requires Named Pipes and Threads on Windows.
> @@ config.mak.uname: ifeq ($(uname_S),MINGW)
> endif
> endif
> @@ config.mak.uname: ifeq ($(uname_S),MINGW)
> EXPAT_NEEDS_XMLPARSE_H = YesPlease
> HAVE_STRINGS_H = YesPlease
> NEEDS_SOCKET = YesPlease
> +@@ config.mak.uname: ifeq ($(uname_S),QNX)
> + NO_PTHREADS = YesPlease
> + NO_STRCASESTR = YesPlease
> + NO_STRLCPY = YesPlease
> ++ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
> + endif
> +
> + ## configure.ac ##
> +@@ configure.ac: fi
> + GIT_CONF_SUBST([ICONV_OMITS_BOM])
> + fi
> +
> ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> ++# prefer using ANSI C signal() over POSIX sigaction()
> ++
> ++AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
> ++ AC_COMPILE_IFELSE(
> ++ [AC_LANG_PROGRAM([#include <signal.h>], [[
> ++ #ifdef SA_RESTART
> ++ #endif
> ++ siginterrupt(SIGCHLD, 1)
> ++ ]])],[ac_cv_siginterrupt=yes],[
> ++ ac_cv_siginterrupt=no
> ++ USE_NON_POSIX_SIGNAL=UnfortunatelyYes
> ++ ]
> ++ )
> ++])
> ++GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])
> ++
> + ## Checks for typedefs, structures, and compiler characteristics.
> + AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
> + #
> +
> + ## meson.build ##
> +@@ meson.build: else
> + build_options_config.set('NO_EXPAT', '1')
> + endif
> +
> ++if compiler.get_define('SA_RESTART', prefix: '#include <signal.h>') == ''
> ++ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'
> ++endif
> ++
> + if not compiler.has_header('sys/select.h')
> + libgit_c_args += '-DNO_SYS_SELECT_H'
> + endif
> 2: 2e8c4643a60 ! 2: 05d945aa1e5 daemon: use sigaction() to install child_handler()
> @@ Commit message
> In a future change, the flags used for processing SIGCHLD will need to
> be updated, which is only possible by using sigaction().
> > - Replace the call, which hs the added benefit of using BSD semantics
> - reliably and therefore not needing the rearming call.
> + Factor out the call to set the signal handler and use sigaction instead
> + of signal for the systems that allow that, which has the added benefit
> + of using BSD semantics reliably and therefore not needing the rearming
> + call.
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > ## daemon.c ##
> -@@ daemon.c: static void child_handler(int signo UNUSED)
> +@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
> + add_child(&cld, addr, addrlen);
> + }
> +
> +-static void child_handler(int signo UNUSED)
> ++static void child_handler(int signo MAYBE_UNUSED)
> + {
> /*
> - * Otherwise empty handler because systemcalls will get interrupted
> - * upon signal receipt
> +- * Otherwise empty handler because systemcalls will get interrupted
> +- * upon signal receipt
> - * SysV needs the handler to be rearmed
> ++ * Otherwise empty handler because systemcalls should get interrupted
> ++ * upon signal receipt.
> */
> - signal(SIGCHLD, child_handler);
> ++#ifdef USE_NON_POSIX_SIGNAL
> ++ /*
> ++ * SysV needs the handler to be rearmed, but this is known
> ++ * to trigger infinite recursion crashes at least in AIX.
> ++ */
> ++ signal(signo, child_handler);
> ++#endif
> }
> > static int set_reuse_addr(int sockfd)
> @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
> + }
> + }
> +
> ++#ifndef USE_NON_POSIX_SIGNAL
> ++
> ++static void set_signal_handler(struct sigaction *psa)
> ++{
> ++ sigemptyset(&psa->sa_mask);
> ++ psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;
> ++ psa->sa_handler = child_handler;
> ++ sigaction(SIGCHLD, psa, NULL);
> ++}
> ++
> ++#else
> ++
> ++static void set_signal_handler(struct sigaction *psa UNUSED)
> ++{
> ++ signal(SIGCHLD, child_handler);
> ++}
> ++
> static int service_loop(struct socketlist *socklist)
> {
> - struct pollfd *pfd;
> + struct sigaction sa;
> + struct pollfd *pfd;
> > CALLOC_ARRAY(pfd, socklist->nr);
> -
> @@ daemon.c: static int service_loop(struct socketlist *socklist)
> pfd[i].events = POLLIN;
> }
> > - signal(SIGCHLD, child_handler);
> -+ sigemptyset(&sa.sa_mask);
> -+ sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> -+ sa.sa_handler = child_handler;
> -+ sigaction(SIGCHLD, &sa, NULL);
> ++ set_signal_handler(&sa);
> > for (;;) {
> check_dead_children();
> 3: a450bdb0066 ! 3: b737e0389df daemon: explicitly allow EINTR during poll()
> @@ Commit message
> might not return with -1 and set errno to EINTR when a signal is
> received.
> > - Since the logic to reap zombie childs relies om those interruptions
> + Since the logic to reap zombie childs relies on those interruptions
> make sure to explicitly disable SA_RESTART around this function.
> > - Add a Makefile flag for portability to systems that don't have the
> - functionality to change those flags or where it is not needed.
> -
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > - ## Makefile ##
> -@@ Makefile: include shared.mak
> - # Define NO_PREAD if you have a problem with pread() system call (e.g.
> - # cygwin1.dll before v1.5.22).
> - #
> -+# Define NO_SIGINTERRUPT if you don't have siginterrupt() or SA_RESTART
> -+# or if your signal(SIGCHLD) implementation doesn't set SA_RESTART.
> -+#
> - # Define NO_SETITIMER if you don't have setitimer()
> - #
> - # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
> -@@ Makefile: ifdef NO_PREAD
> - COMPAT_CFLAGS += -DNO_PREAD
> - COMPAT_OBJS += compat/pread.o
> - endif
> -+ifdef NO_SIGINTERRUPT
> -+ COMPAT_CFLAGS += -DNO_SIGINTERRUPT
> -+endif
> - ifdef NO_FAST_WORKING_DIRECTORY
> - BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
> - endif
> -
> - ## config.mak.uname ##
> -@@ config.mak.uname: ifeq ($(uname_S),Windows)
> - NO_STRTOUMAX = YesPlease
> - NO_MKDTEMP = YesPlease
> - NO_INTTYPES_H = YesPlease
> -+ NO_SIGINTERRUPT = YesPlease
> - CSPRNG_METHOD = rtlgenrandom
> - # VS2015 with UCRT claims that snprintf and friends are C99 compliant,
> - # so we don't need this:
> -@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
> - NO_PREAD = YesPlease
> - NO_MMAP = YesPlease
> - NO_POLL = YesPlease
> -+ NO_SIGINTERRUPT = UnfortunatelyYes
> - NO_INTPTR_T = UnfortunatelyYes
> - CSPRNG_METHOD = openssl
> - SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
> -@@ config.mak.uname: ifeq ($(uname_S),MINGW)
> - NEEDS_LIBICONV = YesPlease
> - NO_STRTOUMAX = YesPlease
> - NO_MKDTEMP = YesPlease
> -+ NO_SIGINTERRUPT = YesPlease
> - NO_SVN_TESTS = YesPlease
> -
> - # The builtin FSMonitor requires Named Pipes and Threads on Windows.
> -@@ config.mak.uname: ifeq ($(uname_S),QNX)
> - NO_PTHREADS = YesPlease
> - NO_STRCASESTR = YesPlease
> - NO_STRLCPY = YesPlease
> -+ NO_SIGINTERRUPT = UnfortunatelyYes
> - endif
> -
> - ## configure.ac ##
> -@@ configure.ac: GIT_CHECK_FUNC(getdelim,
> - [HAVE_GETDELIM=])
> - GIT_CONF_SUBST([HAVE_GETDELIM])
> - #
> -+# Define NO_SIGINTERRUPT if you don't have siginterrupt.
> -+GIT_CHECK_FUNC(siginterrupt,
> -+[NO_SIGINTERRUPT=],
> -+[NO_SIGINTERRUPT=YesPlease])
> -+GIT_CONF_SUBST([NO_SIGINTERRUPT])
> - #
> - # Define NO_MMAP if you want to avoid mmap.
> - #
> -
> ## daemon.c ##
> -@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
> - add_child(&cld, addr, addrlen);
> +@@ daemon.c: static void set_signal_handler(struct sigaction *psa)
> + sigaction(SIGCHLD, psa, NULL);
> }
> > --static void child_handler(int signo UNUSED)
> -+static void child_handler(int signo)
> - {
> - /*
> -- * Otherwise empty handler because systemcalls will get interrupted
> -- * upon signal receipt
> -+ * Empty handler because systemcalls should get interrupted
> -+ * upon signal receipt.
> - */
> -+#ifdef NO_SIGINTERRUPT
> -+ /* SysV needs the handler to be rearmed */
> -+ signal(signo, child_handler);
> -+#endif
> ++static void set_sa_restart(struct sigaction *psa, int enable)
> ++{
> ++ if (enable)
> ++ psa->sa_flags |= SA_RESTART;
> ++ else
> ++ psa->sa_flags &= ~SA_RESTART;
> ++ sigaction(SIGCHLD, psa, NULL);
> ++}
> ++
> + #else
> +
> + static void set_signal_handler(struct sigaction *psa UNUSED)
> +@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)
> + signal(SIGCHLD, child_handler);
> }
> > - static int set_reuse_addr(int sockfd)
> -@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
> -
> ++static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
> ++{
> ++}
> ++
> ++#endif
> ++
> static int service_loop(struct socketlist *socklist)
> {
> -- struct pollfd *pfd;
> -+#ifndef NO_SIGINTERRUPT
> struct sigaction sa;
> -+#endif
> -+ struct pollfd *pfd;
> -
> - CALLOC_ARRAY(pfd, socklist->nr);
> -
> @@ daemon.c: static int service_loop(struct socketlist *socklist)
> - pfd[i].events = POLLIN;
> - }
> -
> -+#ifdef NO_SIGINTERRUPT
> -+ signal(SIGCHLD, child_handler);
> -+#else
> - sigemptyset(&sa.sa_mask);
> - sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> - sa.sa_handler = child_handler;
> - sigaction(SIGCHLD, &sa, NULL);
> -+#endif
> -
> for (;;) {
> check_dead_children();
> > -+#ifndef NO_SIGINTERRUPT
> -+ sa.sa_flags &= ~SA_RESTART;
> -+ sigaction(SIGCHLD, &sa, NULL);
> -+#endif
> ++ set_sa_restart(&sa, 0);
> if (poll(pfd, socklist->nr, -1) < 0) {
> if (errno != EINTR) {
> logerror("Poll failed, resuming: %s",
> @@ daemon.c: static int service_loop(struct socketlist *socklist)
> }
> continue;
> }
> -+#ifndef NO_SIGINTERRUPT
> -+ sa.sa_flags |= SA_RESTART;
> -+ sigaction(SIGCHLD, &sa, NULL);
> -+#endif
> ++ set_sa_restart(&sa, 1);
> > for (size_t i = 0; i < socklist->nr; i++) {
> if (pfd[i].revents & POLLIN) {
> -
> - ## meson.build ##
> -@@ meson.build: checkfuncs = {
> - 'setenv' : ['setenv.c'],
> - 'mkdtemp' : ['mkdtemp.c'],
> - 'initgroups' : [],
> -+ 'siginterrupt' : [],
> - 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
> - 'pread' : ['pread.c'],
> - }
> |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Carlo Marcelo Arenas Belón via GitGitGadget"
<gitgitgadget@gmail.com> writes:
> +@@ Makefile: include shared.mak
> + # when attempting to read from an fopen'ed directory (or even to fopen
> + # it at all).
> + #
> ++# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> ++# prefer to use ANSI C signal() over POSIX sigaction()
> ++#
> ...
> ++ifdef USE_NON_POSIX_SIGNAL
> ++ COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
> ++endif
The new symbol sounds like "POSIX does not have signal(2) but on
this platform we have a usable signal(2), so we use it here", but I
do not think that it is what we want to say (as POSIX inherits this
from ANSI C anyway). More importantly, this "USE_X" sounds as if we
allow builders to set it and magically we stop using sigaction(2),
which is not what is going on. We have tons of calls to both
signal(2) and sigaction(2), and we turn calls to signal(2) we have
in daemon.c to sigaction(2) but on some platforms their sigaction(2)
cannot do what we ask it to do, so we are stuck with signal(2) on
these platforms only for these calls in daemon.c. It may be obvious
to those who develop and review this series, but not for anybody else.
Isn't the situation more like:
We use sigaction(2) everywhere and have been happy with it in
our code, but this topic discovered that on some platforms,
their sigaction(2) does not do XYZ that everybody else's
sigaction(2) does, so on them we need to fall back on the plain
old signal(2) on selected code paths that we need XYZ out of the
signal handling interface.
What is this XYZ that describes the characteristics of
signal/sigaction implementation on these platforms? A name
constructed more like SIGACTION_LACKS_XYZ (hence we have to resort
to signal), possibly with a more appropriate verb than "lack", would
be less confusing.
I think the topic is moving in the right direction with cleaner code
than the previous round. Thanks for investing time in it. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <phillip.wood123@gmail.com> writes:
> On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
>> This series addresses and ambiguity that is at least visible in OpenBSD,
>> where zombie proceses would only be cleared after a new connection is
>> received.
>
> There is still a race where a child that exits after it has been
> checked in check_dead_children() but before we call poll() will not be
> collected until a new connection is received or a child exits while
> we're polling. If we used the self-pipe trick described on the
> select(2) man page [1] we would avoid that race and would not need to
> mess with SA_RESTART and so would not need to introduce
> USE_NON_POSIX_SIGNAL.
>
> Best Wishes
>
> Phillip
>
> [1] https://www.man7.org/linux/man-pages/man2/select.2.html
The principle should apply equally to poll-based service loop, I
presume.
Thanks. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 25/06/2025 17:24, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >> On 25/06/2025 08:35, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
>>> This series addresses and ambiguity that is at least visible in OpenBSD,
>>> where zombie proceses would only be cleared after a new connection is
>>> received.
>>
>> There is still a race where a child that exits after it has been
>> checked in check_dead_children() but before we call poll() will not be
>> collected until a new connection is received or a child exits while
>> we're polling. If we used the self-pipe trick described on the
>> select(2) man page [1] we would avoid that race and would not need to
>> mess with SA_RESTART and so would not need to introduce
>> USE_NON_POSIX_SIGNAL.
>>
>> Best Wishes
>>
>> Phillip
>>
>> [1] https://www.man7.org/linux/man-pages/man2/select.2.html
> > The principle should apply equally to poll-based service loop, I
> presume.
Yes, you create a pipe, add the read end to the set of file descriptors monitored by poll() and write to the other end of the pipe when a signal is received.
Thanks
Phillip
> Thanks.
|
|
This patch series was integrated into seen via e56a707. |
|
This patch series was integrated into seen via ed9d0b5. |
|
This patch series was integrated into seen via 464d990. |
|
This patch series was integrated into seen via 7f8ad08. |
|
This patch series was integrated into seen via c1a593f. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> This series addresses and ambiguity that is at least visible in OpenBSD,
> where zombie proceses would only be cleared after a new connection is
> received.
> > The underlying problem is that when this code was originally introduced,
> SA_RESTART was not widely implemented, and the signal() call usually
> implemented SysV like semantics, at least until it started being
> reimplemented by calling sigaction() internally.
I'm all in favor of using sigaction() but I think the SA_RESTART parts of this series are an unnecessary complication that has the potential to hide bugs as we support platforms without SA_RESTART. Your EINTR patches have all been about efficiency rather than correctness so I think my initial assessment that the existing code handles EINTR correctly was probably true. As I said in my comments on patch 3 I think we can happily drop patches 1 and 4.
Thanks
Phillip
> Changes since v2
> > * Add a new patch 2 that modifies windows' sigaction so there is no more
> need for a fallback
> * Hopefully no more silly mistakes and a variable that finally makes sense
> > Changes since v1
> > * Almost all references to siginterrupt has been removed and a better named
> variable used instead
> * Changes had been abstracted to minimize ifdefs and their introduction
> staged more naturally
> > Carlo Marcelo Arenas Belón (4):
> compat/posix.h: track SA_RESTART fallback
> compat/mingw: allow sigaction(SIGCHLD)
> daemon: use sigaction() to install child_handler()
> daemon: explicitly allow EINTR during poll()
> > Makefile | 5 +++++
> compat/mingw-posix.h | 2 +-
> compat/mingw.c | 4 +++-
> compat/posix.h | 8 ++++++++
> config.mak.uname | 7 ++++---
> configure.ac | 16 ++++++++++++++++
> daemon.c | 33 ++++++++++++++++++++++++++++-----
> meson.build | 4 ++++
> 8 files changed, 69 insertions(+), 10 deletions(-)
> > > base-commit: cb3b40381e1d5ee32dde96521ad7cfd68eb308a6
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2002%2Fcarenas%2Fsiginterrupt-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2002/carenas/siginterrupt-v3
> Pull-Request: https://github.com/git/git/pull/2002
> > Range-diff vs v2:
> > 1: e82b7425bbc ! 1: ae1ca6bb2b2 compat/posix.h: track SA_RESTART fallback
> @@ Makefile: include shared.mak
> # when attempting to read from an fopen'ed directory (or even to fopen
> # it at all).
> #
> -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> -+# prefer to use ANSI C signal() over POSIX sigaction()
> ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
> +#
> # Define OPEN_RETURNS_EINTR if your open() system call may return EINTR
> # when a signal is received (as opposed to restarting).
> @@ Makefile: ifdef FREAD_READS_DIRECTORIES
> COMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES
> COMPAT_OBJS += compat/fopen.o
> endif
> -+ifdef USE_NON_POSIX_SIGNAL
> -+ COMPAT_CFLAGS += -DUSE_NON_POSIX_SIGNAL
> ++ifdef NO_RESTARTABLE_SIGNALS
> ++ COMPAT_CFLAGS += -DNO_RESTARTABLE_SIGNALS
> +endif
> ifdef OPEN_RETURNS_EINTR
> COMPAT_CFLAGS += -DOPEN_RETURNS_EINTR
> @@ compat/posix.h: char *gitdirname(char *);
> + * not on some systems (e.g. NonStop, QNX).
> + */
> +#ifndef SA_RESTART
> -+# define SA_RESTART 0 /* disabled for sigaction() */
> ++# define SA_RESTART 0 /* disabled for sigaction() */
> +#endif
> +
> typedef uintmax_t timestamp_t;
> @@ config.mak.uname: ifeq ($(uname_S),Windows)
> NO_STRTOUMAX = YesPlease
> NO_MKDTEMP = YesPlease
> NO_INTTYPES_H = YesPlease
> -+ USE_NON_POSIX_SIGNAL = YesPlease
> ++ NO_RESTARTABLE_SIGNALS = YesPlease
> CSPRNG_METHOD = rtlgenrandom
> # VS2015 with UCRT claims that snprintf and friends are C99 compliant,
> # so we don't need this:
> @@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
> NO_MMAP = YesPlease
> NO_POLL = YesPlease
> NO_INTPTR_T = UnfortunatelyYes
> -+ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
> ++ NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
> CSPRNG_METHOD = openssl
> SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
> SHELL_PATH = /usr/coreutils/bin/bash
> @@ config.mak.uname: ifeq ($(uname_S),MINGW)
> NEEDS_LIBICONV = YesPlease
> NO_STRTOUMAX = YesPlease
> NO_MKDTEMP = YesPlease
> -+ USE_NON_POSIX_SIGNAL = YesPlease
> ++ NO_RESTARTABLE_SIGNALS = YesPlease
> NO_SVN_TESTS = YesPlease
> > # The builtin FSMonitor requires Named Pipes and Threads on Windows.
> @@ config.mak.uname: ifeq ($(uname_S),QNX)
> NO_PTHREADS = YesPlease
> NO_STRCASESTR = YesPlease
> NO_STRLCPY = YesPlease
> -+ USE_NON_POSIX_SIGNAL = UnfortunatelyYes
> ++ NO_RESTARTABLE_SIGNALS = UnfortunatelyYes
> endif
> > ## configure.ac ##
> @@ configure.ac: fi
> GIT_CONF_SUBST([ICONV_OMITS_BOM])
> fi
> > -+# Define USE_NON_POSIX_SIGNAL if don't have support for SA_RESTART or
> -+# prefer using ANSI C signal() over POSIX sigaction()
> ++# Define NO_RESTARTABLE_SIGNALS if don't have support for SA_RESTART
> +
> +AC_CACHE_CHECK([whether SA_RESTART is supported], [ac_cv_siginterrupt], [
> + AC_COMPILE_IFELSE(
> + [AC_LANG_PROGRAM([#include <signal.h>], [[
> -+ #ifdef SA_RESTART
> -+ #endif
> -+ siginterrupt(SIGCHLD, 1)
> -+ ]])],[ac_cv_siginterrupt=yes],[
> ++ #ifdef SA_RESTART
> ++ restartable signals supported
> ++ #endif
> ++ ]])],[
> + ac_cv_siginterrupt=no
> -+ USE_NON_POSIX_SIGNAL=UnfortunatelyYes
> -+ ]
> ++ NO_RESTARTABLE_SIGNALS=UnfortunatelyYes
> ++ ], [ac_cv_siginterrupt=yes]
> + )
> +])
> -+GIT_CONF_SUBST([USE_NON_POSIX_SIGNAL])
> ++GIT_CONF_SUBST([NO_RESTARTABLE_SIGNALS])
> +
> ## Checks for typedefs, structures, and compiler characteristics.
> AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
> @@ meson.build: else
> endif
> > +if compiler.get_define('SA_RESTART', prefix: '#include <signal.h>') == ''
> -+ libgit_c_args += '-DUSE_NON_POSIX_SIGNAL'
> ++ libgit_c_args += '-DNO_RESTARTABLE_SIGNALS'
> +endif
> +
> if not compiler.has_header('sys/select.h')
> -: ----------- > 2: 3f63479119f compat/mingw: allow sigaction(SIGCHLD)
> 2: 05d945aa1e5 ! 3: c66bda461f4 daemon: use sigaction() to install child_handler()
> @@ Commit message
> In a future change, the flags used for processing SIGCHLD will need to
> be updated, which is only possible by using sigaction().
> > - Factor out the call to set the signal handler and use sigaction instead
> - of signal for the systems that allow that, which has the added benefit
> - of using BSD semantics reliably and therefore not needing the rearming
> - call.
> + Replace signal() with an equivalent invocation of sigaction(), which
> + has the added benefit of using BSD semantics reliably and therefore
> + not needing the rearming call in the signal handler.
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > ## daemon.c ##
> @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
> - add_child(&cld, addr, addrlen);
> - }
> -
> --static void child_handler(int signo UNUSED)
> -+static void child_handler(int signo MAYBE_UNUSED)
> + static void child_handler(int signo UNUSED)
> {
> /*
> - * Otherwise empty handler because systemcalls will get interrupted
> @@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addr
> + * upon signal receipt.
> */
> - signal(SIGCHLD, child_handler);
> -+#ifdef USE_NON_POSIX_SIGNAL
> -+ /*
> -+ * SysV needs the handler to be rearmed, but this is known
> -+ * to trigger infinite recursion crashes at least in AIX.
> -+ */
> -+ signal(signo, child_handler);
> -+#endif
> }
> > static int set_reuse_addr(int sockfd)
> @@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
> - }
> - }
> > -+#ifndef USE_NON_POSIX_SIGNAL
> -+
> -+static void set_signal_handler(struct sigaction *psa)
> -+{
> -+ sigemptyset(&psa->sa_mask);
> -+ psa->sa_flags = SA_NOCLDSTOP | SA_RESTART;
> -+ psa->sa_handler = child_handler;
> -+ sigaction(SIGCHLD, psa, NULL);
> -+}
> -+
> -+#else
> -+
> -+static void set_signal_handler(struct sigaction *psa UNUSED)
> -+{
> -+ signal(SIGCHLD, child_handler);
> -+}
> -+
> static int service_loop(struct socketlist *socklist)
> {
> + struct sigaction sa;
> @@ daemon.c: static int service_loop(struct socketlist *socklist)
> }
> > - signal(SIGCHLD, child_handler);
> -+ set_signal_handler(&sa);
> ++ sigemptyset(&sa.sa_mask);
> ++ sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> ++ sa.sa_handler = child_handler;
> ++ sigaction(SIGCHLD, &sa, NULL);
> > for (;;) {
> check_dead_children();
> 3: b737e0389df ! 4: 851d663be0b daemon: explicitly allow EINTR during poll()
> @@ Commit message
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > ## daemon.c ##
> -@@ daemon.c: static void set_signal_handler(struct sigaction *psa)
> - sigaction(SIGCHLD, psa, NULL);
> +@@ daemon.c: static void socksetup(struct string_list *listen_addr, int listen_port, struct s
> + }
> }
> > ++#ifndef NO_RESTARTABLE_SIGNALS
> ++
> +static void set_sa_restart(struct sigaction *psa, int enable)
> +{
> + if (enable)
> @@ daemon.c: static void set_signal_handler(struct sigaction *psa)
> + sigaction(SIGCHLD, psa, NULL);
> +}
> +
> - #else
> -
> - static void set_signal_handler(struct sigaction *psa UNUSED)
> -@@ daemon.c: static void set_signal_handler(struct sigaction *psa UNUSED)
> - signal(SIGCHLD, child_handler);
> - }
> -
> ++#else
> ++
> +static void set_sa_restart(struct sigaction *psa UNUSED, int enable UNUSED)
> +{
> +}
> |
|
On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this): On Wed, Jul 09, 2025 at 03:12:43PM -0800, Phillip Wood wrote:
> On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> > This series addresses and ambiguity that is at least visible in OpenBSD,
> > where zombie proceses would only be cleared after a new connection is
> > received.
> >
> > The underlying problem is that when this code was originally introduced,
> > SA_RESTART was not widely implemented, and the signal() call usually
> > implemented SysV like semantics, at least until it started being
> > reimplemented by calling sigaction() internally.
>
> I'm all in favor of using sigaction() but I think the SA_RESTART parts of
> this series are an unnecessary complication that has the potential to hide
> bugs as we support platforms without SA_RESTART.
True, but those platforms (except for Windows, which is otherwise not that
relevant as it doesn't fail system calls with EINTR anyway) don't have that
many users and are therefore less likely to uncover any possible issues with
their use cases.
I know patch 4 looks silly, by enabling SA_RESTART just to disable it around
poll(), but it addresses the root cause of the problem stated originally,
which is that we are very likely to have SA_RESTART enabled on SIGCHLD and
relying on the system to excempt poll() from it.
Carlo |
|
This patch series was integrated into seen via 069bdd3. |
A future change will start using sigaction to setup a SIGCHLD signal handler. The current code uses signal(), which returns SIG_ERR (but doesn't seem to set errno) so instruct sigaction() to do the same. A new SA flag will be needed, so copy the one from Cygwinr; note that the sigacgtion() implementation that is provided won't use it, so its value is otherwise irrelevant. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Replace signal() with an equivalent invocation of sigaction(), but make sure to NOT set SA_RESTART so the original code that expects to be interrupted when children complete still works as designed. This change has the added benefit of using BSD signal semantics reliably and therefore not needing the rearming call in the signal handler. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
|
/submit |
|
Submitted as pull.2002.v4.git.git.1752176743.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Thanks, will queue with the following typo-fixes.
I'd appreciate an explicit Ack, even if this version is acceptable
for all those who have helped polish this topic.
I understand that the self-pipe to wake ourselves up is left outside
this topic on purpose, which I agree with.
Thanks, Carlo, for putting this together from weeks' long
discussions, and thanks Phillip for pushing for simpler and smaller
set of changes.
Will queue.
1: 30773a76ce ! 1: ef03aa432a compat/mingw: allow sigaction(SIGCHLD)
@@ Commit message
The current code uses signal(), which returns SIG_ERR (but doesn't
seem to set errno) so instruct sigaction() to do the same.
- A new SA flag will be needed, so copy the one from Cygwinr; note that
- the sigacgtion() implementation that is provided won't use it, so
+ A new SA flag will be needed, so copy the one from Cygwin; note that
+ the sigaction() implementation that is provided won't use it, so
its value is otherwise irrelevant.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
2: 7a33d7a646 = 2: d83e1eef3b daemon: use sigaction() to install child_handler() |
|
User |
|
On the Git mailing list, Carlo Arenas wrote (reply to this): Thanks for the typo fixes
Carlo |
|
This patch series was integrated into seen via f1421ad. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 10/07/2025 22:26, Junio C Hamano wrote:
> Thanks, will queue with the following typo-fixes.
> > I'd appreciate an explicit Ack, even if this version is acceptable
> for all those who have helped polish this topic.
Patch 2 looks good, using sigaction() rather than signal() should reduce the variation in behavior across our unix platforms. I'd be much happier if we set errno on SIGCHLD in patch 1 - the argument in [1] that a non-zero errno might break something because signal() did not set it does not make much sense to me. At the moment it does not matter because there are no callers that check the return value let alone errno but if a future caller does start checking for errors there going to be surprised by errno not getting set.
Thanks
Phillip
[1] <3hrbpiapamvfiuilebjcbcruppz3vukf6mndg62j6gvko2jfs4@ll24s25shcgv>
> I understand that the self-pipe to wake ourselves up is left outside
> this topic on purpose, which I agree with.
> > Thanks, Carlo, for putting this together from weeks' long
> discussions, and thanks Phillip for pushing for simpler and smaller
> set of changes.
> > Will queue.
> > > 1: 30773a76ce ! 1: ef03aa432a compat/mingw: allow sigaction(SIGCHLD)
> @@ Commit message
> The current code uses signal(), which returns SIG_ERR (but doesn't
> seem to set errno) so instruct sigaction() to do the same.
> > - A new SA flag will be needed, so copy the one from Cygwinr; note that
> - the sigacgtion() implementation that is provided won't use it, so
> + A new SA flag will be needed, so copy the one from Cygwin; note that
> + the sigaction() implementation that is provided won't use it, so
> its value is otherwise irrelevant.
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> 2: 7a33d7a646 = 2: d83e1eef3b daemon: use sigaction() to install child_handler()
|
|
This branch is now known as |
|
This patch series was integrated into seen via 451b60d. |
|
This patch series was integrated into seen via e9bfcb5. |
|
There was a status update in the "Cooking" section about the branch Futz with SIGCHLD handling in "git daemon". Will merge to 'next'? source: <pull.2002.v4.git.git.1752176743.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch Futz with SIGCHLD handling in "git daemon". Will merge to 'next'? source: <pull.2002.v4.git.git.1752176743.gitgitgadget@gmail.com> |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <phillip.wood123@gmail.com> writes:
> much happier if we set errno on SIGCHLD in patch 1 - the argument in
> [1] that a non-zero errno might break something because signal() did
> not set it does not make much sense to me.
Not to me either.
> At the moment it does not
> matter because there are no callers that check the return value let
> alone errno but if a future caller does start checking for errors
> there going to be surprised by errno not getting set.
True, again.
Let's queue this round and then patch the errno issue up on top
after the dust settles. "might break something" may then happen,
at which time it is easier to see where that breakage came from,
and we can go from there.
Thanks, all.
|
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 14/07/2025 22:52, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> > Let's queue this round and then patch the errno issue up on top
> after the dust settles. "might break something" may then happen,
> at which time it is easier to see where that breakage came from,
> and we can go from there.
That sounds good to me
Thanks
Phillip |
|
This patch series was integrated into seen via cdc7d1e. |
|
This patch series was integrated into next via a357435. |
|
This patch series was integrated into seen via e61fbc0. |
|
This patch series was integrated into seen via 52d1625. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series addresses and ambiguity that is at least visible in OpenBSD, where zombie proceses would only be cleared after a new connection is received.
The underlying problem is that when this code was originally introduced, SA_RESTART was not widely implemented, and the
signal()call usually implemented SysV like semantics, at least until it started being reimplemented by callingsigaction()internally.Changes since v3
Changes since v2
Changes since v1
cc: Carlo Marcelo Arenas Belón carenas@gmail.com
cc: Chris Torek chris.torek@gmail.com
cc: Phillip Wood phillip.wood123@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com