libfortran/90038 Reap dead children when wait=.false.

Message ID 20190519104059.19382-1-blomqvist.janne@gmail.com
State New
Headers show
Series
  • libfortran/90038 Reap dead children when wait=.false.
Related show

Commit Message

Janne Blomqvist May 19, 2019, 10:40 a.m.
When using posix_spawn or fork to launch a child process, the parent
needs to wait for the child, otherwise the dead child is left as a
zombie process. For this purpose one can install a signal handler for
SIGCHLD.

2019-05-19  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/90038
	* intrinsics/execute_command_line (sigchld_handler): New function.
        (execute_command_line): Install handler for SIGCHLD.
        * configure.ac: Check for presence of sigaction and waitpid.
        * config.h.in: Regenerated.
        * configure: Regenerated.

Regtested on x86_64-pc-linux-gnu, Ok for trunk?
---
 libgfortran/configure.ac                      |  2 +-
 libgfortran/intrinsics/execute_command_line.c | 25 +++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Steve Kargl May 19, 2019, 4:15 p.m. | #1
On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote:
>  

> +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID)

> +      static bool sig_init_saved;

> +      bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED);

> +      if (!sig_init)

> +	{

> +	  struct sigaction sa;

> +	  sa.sa_handler = &sigchld_handler;

> +	  sigemptyset(&sa.sa_mask);

> +	  sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;

> +	  sigaction(SIGCHLD, &sa, 0);

> +	  __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED);

> +	}

> +#endif


Where do the prototypes for __atomic_load_n and __atomic_store_n
come from?  On FreeBSD, it seems these are in stdatomic.h.  Do
we need a HAVE_ATOMIC to include the header?

On a slightly different note, the nonstandard SYSTEM intrinsic
uses system(3) to execute a command.  I believe that it will
leave zombies/orphaned children if a process is interrupted.
Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE.

-- 
Steve
Janne Blomqvist May 19, 2019, 6:10 p.m. | #2
On Sun, May 19, 2019 at 7:15 PM Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
>

> On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote:

> >

> > +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID)

> > +      static bool sig_init_saved;

> > +      bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED);

> > +      if (!sig_init)

> > +     {

> > +       struct sigaction sa;

> > +       sa.sa_handler = &sigchld_handler;

> > +       sigemptyset(&sa.sa_mask);

> > +       sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;

> > +       sigaction(SIGCHLD, &sa, 0);

> > +       __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED);

> > +     }

> > +#endif

>

> Where do the prototypes for __atomic_load_n and __atomic_store_n

> come from?  On FreeBSD, it seems these are in stdatomic.h.  Do

> we need a HAVE_ATOMIC to include the header?


They are GCC atomic builtins, they are always available, no need to
include a header:
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html

> On a slightly different note, the nonstandard SYSTEM intrinsic

> uses system(3) to execute a command.  I believe that it will

> leave zombies/orphaned children if a process is interrupted.

> Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE.


I believe any competent implementation of system() would wait for the
child process. Since system() is synchronous, it can do the waiting
inline without having to use a signal handler.

-- 
Janne Blomqvist
Steve Kargl May 19, 2019, 6:26 p.m. | #3
On Sun, May 19, 2019 at 09:10:57PM +0300, Janne Blomqvist wrote:
> On Sun, May 19, 2019 at 7:15 PM Steve Kargl

> <sgk@troutmask.apl.washington.edu> wrote:

> >

> > On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote:

> > >

> > > +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID)

> > > +      static bool sig_init_saved;

> > > +      bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED);

> > > +      if (!sig_init)

> > > +     {

> > > +       struct sigaction sa;

> > > +       sa.sa_handler = &sigchld_handler;

> > > +       sigemptyset(&sa.sa_mask);

> > > +       sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;

> > > +       sigaction(SIGCHLD, &sa, 0);

> > > +       __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED);

> > > +     }

> > > +#endif

> >

> > Where do the prototypes for __atomic_load_n and __atomic_store_n

> > come from?  On FreeBSD, it seems these are in stdatomic.h.  Do

> > we need a HAVE_ATOMIC to include the header?

> 

> They are GCC atomic builtins, they are always available, no need to

> include a header:

> https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html


Thanks for the clarification.  OK for trunk.

> > On a slightly different note, the nonstandard SYSTEM intrinsic

> > uses system(3) to execute a command.  I believe that it will

> > leave zombies/orphaned children if a process is interrupted.

> > Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE.

> 

> I believe any competent implementation of system() would wait for the

> child process. Since system() is synchronous, it can do the waiting

> inline without having to use a signal handler.


I'll need to check.  I recall that

program foo
   call system("Something_that_takes_along_time_to_execute")
end program foo

gfortran -o foo foo.f90

% foo
^C

Results in termination of the parent foo, and
Something_that_takes_along_time_to_execute is orphaned and
continues to run.  It would probably be better to deliver
SIGKILL to child.  I suppose that that is for another day.

-- 
Steve
Janne Blomqvist May 19, 2019, 7:51 p.m. | #4
On Sun, May 19, 2019 at 9:26 PM Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
>

> On Sun, May 19, 2019 at 09:10:57PM +0300, Janne Blomqvist wrote:

> > On Sun, May 19, 2019 at 7:15 PM Steve Kargl

> > <sgk@troutmask.apl.washington.edu> wrote:

> > >

> > > On Sun, May 19, 2019 at 01:40:59PM +0300, Janne Blomqvist wrote:

> > > >

> > > > +#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID)

> > > > +      static bool sig_init_saved;

> > > > +      bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED);

> > > > +      if (!sig_init)

> > > > +     {

> > > > +       struct sigaction sa;

> > > > +       sa.sa_handler = &sigchld_handler;

> > > > +       sigemptyset(&sa.sa_mask);

> > > > +       sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;

> > > > +       sigaction(SIGCHLD, &sa, 0);

> > > > +       __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED);

> > > > +     }

> > > > +#endif

> > >

> > > Where do the prototypes for __atomic_load_n and __atomic_store_n

> > > come from?  On FreeBSD, it seems these are in stdatomic.h.  Do

> > > we need a HAVE_ATOMIC to include the header?

> >

> > They are GCC atomic builtins, they are always available, no need to

> > include a header:

> > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html

>

> Thanks for the clarification.  OK for trunk.


Thanks, committed as r271384.

> > > On a slightly different note, the nonstandard SYSTEM intrinsic

> > > uses system(3) to execute a command.  I believe that it will

> > > leave zombies/orphaned children if a process is interrupted.

> > > Perhap, SYSTEM should be reworked to use your EXECUTE_COMMAND_LINE.

> >

> > I believe any competent implementation of system() would wait for the

> > child process. Since system() is synchronous, it can do the waiting

> > inline without having to use a signal handler.

>

> I'll need to check.  I recall that

>

> program foo

>    call system("Something_that_takes_along_time_to_execute")

> end program foo

>

> gfortran -o foo foo.f90

>

> % foo

> ^C

>

> Results in termination of the parent foo, and

> Something_that_takes_along_time_to_execute is orphaned and

> continues to run.  It would probably be better to deliver

> SIGKILL to child.  I suppose that that is for another day.


I guess the expected behavior is that is the parent dies, the child
gets orphaned and thus re-parented to PID 1 (that is, init) which
takes care of wait()'ing for it so it's not left as a zombie when it
dies. Not sure it's possible to change this is a robust and portable
way. But if someone figures it out, we can think about it then.



-- 
Janne Blomqvist

Patch

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 8fd5a1a30a9..7cfce28ab69 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -314,7 +314,7 @@  if test "${hardwire_newlib:-0}" -eq 1; then
 else
    AC_CHECK_FUNCS_ONCE(getrusage times mkstemp strtof strtold snprintf \
    ftruncate chsize chdir getentropy getlogin gethostname kill link symlink \
-   sleep ttyname \
+   sleep ttyname sigaction waitpid \
    alarm access fork posix_spawn setmode fcntl writev \
    gettimeofday stat fstat lstat getpwuid vsnprintf dup \
    getcwd localtime_r gmtime_r getpwuid_r ttyname_r clock_gettime \
diff --git a/libgfortran/intrinsics/execute_command_line.c b/libgfortran/intrinsics/execute_command_line.c
index 2ef2324b243..1a471632172 100644
--- a/libgfortran/intrinsics/execute_command_line.c
+++ b/libgfortran/intrinsics/execute_command_line.c
@@ -36,6 +36,9 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include <spawn.h>
 extern char **environ;
 #endif
+#if defined(HAVE_POSIX_SPAWN) || defined(HAVE_FORK)
+#include <signal.h>
+#endif
 
 enum { EXEC_SYNCHRONOUS = -2, EXEC_NOERROR = 0, EXEC_SYSTEMFAILED,
        EXEC_CHILDFAILED, EXEC_INVALIDCOMMAND };
@@ -62,6 +65,14 @@  set_cmdstat (int *cmdstat, int value)
 }
 
 
+#if defined(HAVE_WAITPID) && defined(HAVE_SIGACTION)
+static void
+sigchld_handler (int signum __attribute__((unused)))
+{
+  while (waitpid ((pid_t)(-1), NULL, WNOHANG) > 0) {}
+}
+#endif
+
 static void
 execute_command_line (const char *command, bool wait, int *exitstat,
 		      int *cmdstat, char *cmdmsg,
@@ -82,6 +93,20 @@  execute_command_line (const char *command, bool wait, int *exitstat,
 
       set_cmdstat (cmdstat, EXEC_NOERROR);
 
+#if defined(HAVE_SIGACTION) && defined(HAVE_WAITPID)
+      static bool sig_init_saved;
+      bool sig_init = __atomic_load_n (&sig_init_saved, __ATOMIC_RELAXED);
+      if (!sig_init)
+	{
+	  struct sigaction sa;
+	  sa.sa_handler = &sigchld_handler;
+	  sigemptyset(&sa.sa_mask);
+	  sa.sa_flags = SA_RESTART | SA_NOCLDSTOP;
+	  sigaction(SIGCHLD, &sa, 0);
+	  __atomic_store_n (&sig_init_saved, true, __ATOMIC_RELAXED);
+	}
+#endif
+
 #ifdef HAVE_POSIX_SPAWN
       const char * const argv[] = {"sh", "-c", cmd, NULL};
       if (posix_spawn (&pid, "/bin/sh", NULL, NULL,