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

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

[Clint Adams]

On Wed, Aug 18, 2010 at 02:51:48PM +1200, Victor Villa wrote:
> After running a process in the background (by putting "&" at the end of the command line) the output of "jobs" correctly shows 
> the process as "running". If I send the process a STOP signal with "kill -STOP", the process stops and "jobs" shows the 
> process as "suspended (signal)".  The problem is that after sending a CONT signal to the process (with kill -CONT), 
> "jobs" still shows the process as suspended, even though the process resumes (if it's a media file I can hear it 
> playing, for example.
> 
> The processes I used to test this were "ping -i 4 -f localhost" and mpg321.  I tried the same using bash and the status
> are correctly updated. I tried also "/bin/kill" but the same thing happens.

Your job table isn't being updated when you send the STOP/CONT signals externally.

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

[Peter Stephenson]

On Wed, 18 Aug 2010 18:29:59 +0000
Clint Adams <schizo@debian.org> wrote:
> On Wed, Aug 18, 2010 at 02:51:48PM +1200, Victor Villa wrote:
> > After running a process in the background (by putting "&" at the end
> > of the command line) the output of "jobs" correctly shows the
> > process as "running". If I send the process a STOP signal with "kill
> > -STOP", the process stops and "jobs" shows the process as "suspended
> > (signal)".  The problem is that after sending a CONT signal to the
> > process (with kill -CONT), "jobs" still shows the process as
> > suspended, even though the process resumes (if it's a media file I
> > can hear it playing, for example.

Part of the problem is that we don't update the process status properly
when sending a SIGCONT.  That's fairly easy to fix, and it sounds like
it addresses the immediate issue.

However, we ought to be cleverer, since we don't get a signal when a
child gets a SIGCONT from outside the shell, which is perfectly
possible.  To handle that we'd need to use wait3() or waitpid() with
WNOHANG to see if the job was still stopped.  That's still fairly
straightforward, the problem is that if the job has just exited we then
need to update the job status as if we were in the SIGCHLD handler, and
that needs more thought to do it properly.  Probably not *that*
difficult.  I don't suppose anyone has an urge to learn about the signal
code?

Index: Src/jobs.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/jobs.c,v
retrieving revision 1.77
diff -p -u -r1.77 jobs.c
--- Src/jobs.c	31 Jul 2010 22:03:41 -0000	1.77
+++ Src/jobs.c	18 Aug 2010 19:58:50 -0000
@@ -2217,7 +2217,7 @@ bin_kill(char *nam, char **argv, UNUSED(
 	    signal. */
 	    if (jobtab[p].stat & STAT_STOPPED) {
 		if (sig == SIGCONT)
-		    jobtab[p].stat &= ~STAT_STOPPED;
+		    makerunning(jobtab + p);
 		if (sig != SIGKILL && sig != SIGCONT && sig != SIGTSTP
 		    && sig != SIGTTOU && sig != SIGTTIN && sig != SIGSTOP)
 		    killjb(jobtab + p, SIGCONT);
@@ -2225,9 +2225,19 @@ bin_kill(char *nam, char **argv, UNUSED(
 	} else if (!isanum(*argv)) {
 	    zwarnnam("kill", "illegal pid: %s", *argv);
 	    returnval++;
-	} else if (kill(atoi(*argv), sig) == -1) {
-	    zwarnnam("kill", "kill %s failed: %e", *argv, errno);
-	    returnval++;
+	} else {
+	    int pid = atoi(*argv);
+	    if (kill(pid, sig) == -1) {
+		zwarnnam("kill", "kill %s failed: %e", *argv, errno);
+		returnval++;
+	    } else if (sig == SIGCONT) {
+		Job jn;
+		Process pn;
+		if (findproc(pid, &jn, &pn, 0)) {
+		    if (WIFSTOPPED(pn->status))
+			pn->status = SP_RUNNING;
+		}
+	    }
 	}
     }
     unqueue_signals();

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

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

[Bart Schaefer]

On Aug 18,  9:09pm, Peter Stephenson wrote:
}
} +	    } else if (sig == SIGCONT) {
} +		Job jn;
} +		Process pn;
} +		if (findproc(pid, &jn, &pn, 0)) {
} +		    if (WIFSTOPPED(pn->status))
} +			pn->status = SP_RUNNING;
} +		}
} +	    }

Hmm, are we really guaranteed that the job has started running again
just because we killed it with SIGCONT?  E.g., if the reason it was
stopped is because it got a TTIN or TTOU, is there a possible race
here because it's just going to immediately stop again?

-- 

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

[Peter Stephenson]

On Thu, 19 Aug 2010 21:22:51 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Aug 18,  9:09pm, Peter Stephenson wrote:
> }
> } +	    } else if (sig == SIGCONT) {
> } +		Job jn;
> } +		Process pn;
> } +		if (findproc(pid, &jn, &pn, 0)) {
> } +		    if (WIFSTOPPED(pn->status))
> } +			pn->status = SP_RUNNING;
> } +		}
> } +	    }
> 
> Hmm, are we really guaranteed that the job has started running again
> just because we killed it with SIGCONT?  E.g., if the reason it was
> stopped is because it got a TTIN or TTOU, is there a possible race
> here because it's just going to immediately stop again?

Well, signals are queued at this point, so if there's another stop signal
it should be handled after marking the process as continued.

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.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

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

[Peter Stephenson]

On Fri, 20 Aug 2010 09:55:27 +0100
Peter Stephenson <Peter.Stephenson@csr.com> 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@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

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

[Jilles Tjoelker]

> [continuing a stopped background job using kill is not reflected in the
> output of jobs]

I think hooking into the kill builtin is the wrong way to fix this.
Instead, use the facilities provided by modern systems which can notify
you if a child process continues. These are si_code CLD_CONTINUED for
SIGCHLD and the WCONTINUED flag and WIFCONTINUED() macro for waitpid().
Some systems do not provide these, and may not even provide queuing and
siginfo for SIGCHLD, so the latter approach seems best. The WCONTINUED
stuff can then be #ifdef'ed out for systems that do not support it.

-- 
Jilles Tjoelker

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

[Bart Schaefer]

On Aug 20, 12:14am, Jilles Tjoelker wrote:
}
} > [continuing a stopped background job using kill is not reflected in the
} > output of jobs]
} 
} I think hooking into the kill builtin is the wrong way to fix this.
} Instead, use the facilities provided by modern systems which can notify
} you if a child process continues. These are si_code CLD_CONTINUED for
} SIGCHLD and the WCONTINUED flag and WIFCONTINUED() macro for waitpid().

Zsh *soes* use those facilities.

The problem (if I understand the thread so far correctly) is, in order
to use those facilities, zsh has to wait for the job, i.e., call one of
waitpid() or the like.  But the shell doesn't just sit around all day
waiting for background jobs or polling the ones that are stopped to see
if they spontaneously started again -- in fact the whole point is that
it does NOT wait for background jobs.

So unless the OS sends a SIGCHLD when the background job changes state,
zsh isn't aware that it ought to check on the child status.  There is
a SIGCHLD sent to the parent when the background job stops, but that
signal is NOT sent when the child resumes running again.

You can even see this for yourself by installing a handler, e.g.,

    trap "print CHILD" SIGCHLD

Now run something in the background, kill it with -STOP/-CONT from some
other shell, and watch when CHILD is printed (or isn't).

-- 

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

[Jilles Tjoelker]

On Thu, Aug 19, 2010 at 07:36:30PM -0700, Bart Schaefer wrote:
> On Aug 20, 12:14am, Jilles Tjoelker wrote:
> }
> } > [continuing a stopped background job using kill is not reflected in the
> } > output of jobs]
> } 
> } I think hooking into the kill builtin is the wrong way to fix this.
> } Instead, use the facilities provided by modern systems which can notify
> } you if a child process continues. These are si_code CLD_CONTINUED for
> } SIGCHLD and the WCONTINUED flag and WIFCONTINUED() macro for waitpid().

> Zsh *soes* use those facilities.

> The problem (if I understand the thread so far correctly) is, in order
> to use those facilities, zsh has to wait for the job, i.e., call one of
> waitpid() or the like.  But the shell doesn't just sit around all day
> waiting for background jobs or polling the ones that are stopped to see
> if they spontaneously started again -- in fact the whole point is that
> it does NOT wait for background jobs.

> So unless the OS sends a SIGCHLD when the background job changes state,
> zsh isn't aware that it ought to check on the child status.  There is
> a SIGCHLD sent to the parent when the background job stops, but that
> signal is NOT sent when the child resumes running again.

> You can even see this for yourself by installing a handler, e.g.,

>     trap "print CHILD" SIGCHLD

> Now run something in the background, kill it with -STOP/-CONT from some
> other shell, and watch when CHILD is printed (or isn't).

It seems that CHLD traps are only executed when a background job
terminates. A simple command like '/bin/ls /' doesn't trigger it, and a
background job consisting of multiple processes like
'sleep 2 | sleep 1 &' triggers it only once. However, in those cases a
SIGCHLD signal is still sent (checked with ktrace(1) on FreeBSD
8-stable). FreeBSD 8 also sends SIGCHLD when a child process continues.

With ktrace, I also saw that zsh does not use the WCONTINUED flag.

The special handling of CHLD traps seems documented behaviour. From
the man page:
] When the monitor mode is on, each background job that completes
] triggers any trap set for CHLD.
ksh93 behaves similarly.

-- 
Jilles Tjoelker