Re: Bug#593426: zsh: Status of background jobs not updated

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On Fri, 20 Aug 2010 09:55:27 +0100
Peter Stephenson <Peter.Stephenson@xxxxxxx> wrote:
> But it would still be better to check the status by waiting at appropriate
> points, which needs the fix to take care of the case that the child has
> actually exited I mentioned before, which needs a lot of care as it's
> dealing with some basic and race-prone stuff.

Turns out not to be so hard, and not so dangerous, I think.

The chunk for handling SIGCHLD lifts cleanly out into a function (that's
all I've done in signals.h).  It's already safe about checking whether
there are multiple processes with state changes, or no processes left to
handle.  Given that this runs asynchronously anyway, running it at
judicious points where signals are queued (so the same code isn't going
to get run simultaneously by an interrupt) should be safe, and if it
isn't it's probably because of some pre-existing problem rather than
something I've introduced.

The remaining part is to persuade wait to tell us about continued
processes and for update_job() to know what to do about them.

I've therefore conditionally taken out the code that updates continued
statuses after sending them the signals.

This should make the discussion in the other part of this thread moot.

Index: Src/jobs.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/jobs.c,v
retrieving revision 1.78
diff -p -u -r1.78 jobs.c
--- Src/jobs.c	18 Aug 2010 21:21:17 -0000	1.78
+++ Src/jobs.c	20 Aug 2010 23:57:24 -0000
@@ -331,11 +361,20 @@ update_job(Job jn)
     int val = 0, status = 0;
     int somestopped = 0, inforeground = 0;
 
-    for (pn = jn->auxprocs; pn; pn = pn->next)
+    for (pn = jn->auxprocs; pn; pn = pn->next) {
+#ifdef WIFCONTINUED
+	if (WIFCONTINUED(pn->status))
+	    pn->status = SP_RUNNING;
+#endif
 	if (pn->status == SP_RUNNING)
 	    return;
+    }
 
     for (pn = jn->procs; pn; pn = pn->next) {
+#ifdef WIFCONTINUED
+	if (WIFCONTINUED(pn->status))
+	    pn->status = SP_RUNNING;
+#endif
 	if (pn->status == SP_RUNNING)      /* some processes in this job are running       */
 	    return;                        /* so no need to update job table entry         */
 	if (WIFSTOPPED(pn->status))        /* some processes are stopped                   */
@@ -1793,6 +1833,14 @@ bin_fg(char *name, char **argv, Options 
     }
 
     queue_signals();
+    /*
+     * In case any processes changed state recently, wait for them.
+     * This updates stopped processes (but we should have been
+     * signalled about those, up to inevitable races), and also
+     * continued processes if that feature is available.
+     */
+    wait_for_processes();
+
     /* If necessary, update job table. */
     if (unset(NOTIFY))
 	scanjobs();
@@ -2216,8 +2264,11 @@ bin_kill(char *nam, char **argv, UNUSED(
 	    job, and send the job a SIGCONT if sending it a non-stopping
 	    signal. */
 	    if (jobtab[p].stat & STAT_STOPPED) {
+#ifndef WIFCONTINUED
+		/* With WIFCONTINUED we find this out properly */
 		if (sig == SIGCONT)
 		    makerunning(jobtab + p);
+#endif
 		if (sig != SIGKILL && sig != SIGCONT && sig != SIGTSTP
 		    && sig != SIGTTOU && sig != SIGTTIN && sig != SIGSTOP)
 		    killjb(jobtab + p, SIGCONT);
@@ -2230,14 +2281,18 @@ bin_kill(char *nam, char **argv, UNUSED(
 	    if (kill(pid, sig) == -1) {
 		zwarnnam("kill", "kill %s failed: %e", *argv, errno);
 		returnval++;
-	    } else if (sig == SIGCONT) {
+	    } 
+#ifndef WIFCONTINUED
+	    else if (sig == SIGCONT) {
 		Job jn;
 		Process pn;
+		/* With WIFCONTINUED we find this out properly */
 		if (findproc(pid, &jn, &pn, 0)) {
 		    if (WIFSTOPPED(pn->status))
 			pn->status = SP_RUNNING;
 		}
 	    }
+#endif
 	}
     }
     unqueue_signals();
Index: Src/signals.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/signals.c,v
retrieving revision 1.58
diff -p -u -r1.58 signals.c
--- Src/signals.c	12 May 2010 10:07:01 -0000	1.58
+++ Src/signals.c	20 Aug 2010 23:57:24 -0000
@@ -400,6 +400,129 @@ signal_suspend(UNUSED(int sig), int wait
 /**/
 int last_signal;
 
+/*
+ * Wait for any processes that have changed state.
+ *
+ * The main use for this is in the SIGCHLD handler.  However,
+ * we also use it to pick up status changes of jobs when
+ * updating jobs.
+ */
+/**/
+void
+wait_for_processes(void)
+{
+    /* keep WAITING until no more child processes to reap */
+    for (;;) {
+	/* save the errno, since WAIT may change it */
+	int old_errno = errno;
+	int status;
+	Job jn;
+	Process pn;
+	pid_t pid;
+	pid_t *procsubpid = &cmdoutpid;
+	int *procsubval = &cmdoutval;
+	int cont = 0;
+	struct execstack *es = exstack;
+
+	/*
+	 * Reap the child process.
+	 * If we want usage information, we need to use wait3.
+	 */
+#if defined(HAVE_WAIT3) || defined(HAVE_WAITPID)
+# ifdef WCONTINUED
+# define WAITFLAGS (WNOHANG|WUNTRACED|WCONTINUED)
+# else
+# define WAITFLAGS (WNOHANG|WUNTRACED)
+# endif
+#endif
+#ifdef HAVE_WAIT3
+# ifdef HAVE_GETRUSAGE
+	struct rusage ru;
+
+	pid = wait3((void *)&status, WAITFLAGS, &ru);
+# else
+	pid = wait3((void *)&status, WAITFLAGS, NULL);
+# endif
+#else
+# ifdef HAVE_WAITPID
+	pid = waitpid(-1, &status, WAITFLAGS);
+# else
+	pid = wait(&status);
+# endif
+#endif
+
+	if (!pid)  /* no more children to reap */
+	    break;
+
+	/* check if child returned was from process substitution */
+	for (;;) {
+	    if (pid == *procsubpid) {
+		*procsubpid = 0;
+		if (WIFSIGNALED(status))
+		    *procsubval = (0200 | WTERMSIG(status));
+		else
+		    *procsubval = WEXITSTATUS(status);
+		use_cmdoutval = 1;
+		get_usage();
+		cont = 1;
+		break;
+	    }
+	    if (!es)
+		break;
+	    procsubpid = &es->cmdoutpid;
+	    procsubval = &es->cmdoutval;
+	    es = es->next;
+	}
+	if (cont)
+	    continue;
+
+	/* check for WAIT error */
+	if (pid == -1) {
+	    if (errno != ECHILD)
+		zerr("wait failed: %e", errno);
+	    /* WAIT changed errno, so restore the original */
+	    errno = old_errno;
+	    break;
+	}
+
+	/*
+	 * Find the process and job containing this pid and
+	 * update it.
+	 */
+	pn = NULL;
+	if (findproc(pid, &jn, &pn, 0)) {
+#if defined(HAVE_WAIT3) && defined(HAVE_GETRUSAGE)
+	    struct timezone dummy_tz;
+	    gettimeofday(&pn->endtime, &dummy_tz);
+	    pn->status = status;
+	    pn->ti = ru;
+#else
+	    update_process(pn, status);
+#endif
+	    update_job(jn);
+	} else if (findproc(pid, &jn, &pn, 1)) {
+	    pn->status = status;
+	    update_job(jn);
+	} else {
+	    /* If not found, update the shell record of time spent by
+	     * children in sub processes anyway:  otherwise, this
+	     * will get added on to the next found process that
+	     * terminates.
+	     */
+	    get_usage();
+	}
+	/*
+	 * Remember the status associated with $!, so we can
+	 * wait for it even if it's exited.  This value is
+	 * only used if we can't find the PID in the job table,
+	 * so it doesn't matter that the value we save here isn't
+	 * useful until the process has exited.
+	 */
+	if (pn != NULL && pid == lastpid && lastpid_status != -1L)
+	    lastpid_status = lastval2;
+    }
+}
+
 /* the signal handler */
  
 /**/
@@ -458,110 +581,7 @@ zhandler(int sig)
  
     switch (sig) {
     case SIGCHLD:
-
-	/* keep WAITING until no more child processes to reap */
-	for (;;) {
-	    /* save the errno, since WAIT may change it */
-	    int old_errno = errno;
-	    int status;
-	    Job jn;
-	    Process pn;
-	    pid_t pid;
-	    pid_t *procsubpid = &cmdoutpid;
-	    int *procsubval = &cmdoutval;
-	    int cont = 0;
-	    struct execstack *es = exstack;
-
-	    /*
-	     * Reap the child process.
-	     * If we want usage information, we need to use wait3.
-	     */
-#ifdef HAVE_WAIT3
-# ifdef HAVE_GETRUSAGE
-	    struct rusage ru;
-
-	    pid = wait3((void *)&status, WNOHANG|WUNTRACED, &ru);
-# else
-	    pid = wait3((void *)&status, WNOHANG|WUNTRACED, NULL);
-# endif
-#else
-# ifdef HAVE_WAITPID
-	    pid = waitpid(-1, &status, WNOHANG|WUNTRACED);
-# else
-	    pid = wait(&status);
-# endif
-#endif
-
-	    if (!pid)  /* no more children to reap */
-		break;
-
-	    /* check if child returned was from process substitution */
-	    for (;;) {
-		if (pid == *procsubpid) {
-		    *procsubpid = 0;
-		    if (WIFSIGNALED(status))
-			*procsubval = (0200 | WTERMSIG(status));
-		    else
-			*procsubval = WEXITSTATUS(status);
-		    use_cmdoutval = 1;
-		    get_usage();
-		    cont = 1;
-		    break;
-		}
-		if (!es)
-		    break;
-		procsubpid = &es->cmdoutpid;
-		procsubval = &es->cmdoutval;
-		es = es->next;
-	    }
-	    if (cont)
-		continue;
-
-	    /* check for WAIT error */
-	    if (pid == -1) {
-		if (errno != ECHILD)
-		    zerr("wait failed: %e", errno);
-		/* WAIT changed errno, so restore the original */
-		errno = old_errno;
-		break;
-	    }
-
-	    /*
-	     * Find the process and job containing this pid and
-	     * update it.
-	     */
-	    pn = NULL;
-	    if (findproc(pid, &jn, &pn, 0)) {
-#if defined(HAVE_WAIT3) && defined(HAVE_GETRUSAGE)
-		struct timezone dummy_tz;
-		gettimeofday(&pn->endtime, &dummy_tz);
-		pn->status = status;
-		pn->ti = ru;
-#else
-		update_process(pn, status);
-#endif
-		update_job(jn);
-	    } else if (findproc(pid, &jn, &pn, 1)) {
-		pn->status = status;
-		update_job(jn);
-	    } else {
-		/* If not found, update the shell record of time spent by
-		 * children in sub processes anyway:  otherwise, this
-		 * will get added on to the next found process that
-		 * terminates.
-		 */
-		get_usage();
-	    }
-	    /*
-	     * Remember the status associated with $!, so we can
-	     * wait for it even if it's exited.  This value is
-	     * only used if we can't find the PID in the job table,
-	     * so it doesn't matter that the value we save here isn't
-	     * useful until the process has exited.
-	     */
-	    if (pn != NULL && pid == lastpid && lastpid_status != -1L)
-		lastpid_status = lastval2;
-	}
+	wait_for_processes();
         break;
  
     case SIGHUP:

-- 
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/