Job control bug with pws-25

Job control bug with pws-25

[Peter Stephenson]

Actually, as soon as I had I chance to look I found some more job control
bugs (hope I got all the right patches into pws-25).  If you do

while true; do zcat; done

and hit ^C repeatedly, the zcat gets killed but the loop continues.
Eventually, however, you catch the shell when it's not running zcat and the
loop exits.  Doing this a few times gives the message

zsh: job table full

and you can't run any more external jobs, so it's not getting deleted
properly.


Secondly (and this really makes me suspect I've missed a patch):

% fn() { sleep 2; print foo; }
% fn
^Zzsh: 13719 suspended  fn
% bg
[1]  - continued  fn
<after a while>
% jobs -l
[1]  - 13719 running    fn
% fg %1
^C^C^C^C^C^C^C^Z^Z^Z^Z^Z^Z^Z^Z

no effect.  Shell has to killed with -9 from somewhere else.  Note that it
has forked, as expected, and doesn't seem to be doing anything.

     pws 13719 26505   0 14:15:24  pts/0  0:00 ./zsh 
     pws 26505 41151   0 14:15:16  pts/0  0:00 ./zsh 

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

Re: Job control bug with pws-25

[Geoff Wing]

Peter Stephenson <pws@ibmth.df.unipi.it> typed:
:Actually, as soon as I had I chance to look I found some more job control
:bugs (hope I got all the right patches into pws-25).  If you do
:while true; do zcat; done
:and hit ^C repeatedly, the zcat gets killed but the loop continues.

Didn't really have a problem with this.  I was able to stop it with little
grief.

:Eventually, however, you catch the shell when it's not running zcat and the
:loop exits.  Doing this a few times gives the message
:zsh: job table full

Never saw this.

:and you can't run any more external jobs, so it's not getting deleted
:properly.


:Secondly (and this really makes me suspect I've missed a patch):
:% fn() { sleep 2; print foo; }
:% fn
:^Zzsh: 13719 suspended  fn
:% bg
:[1]  - continued  fn
:<after a while>
:% jobs -l
:[1]  - 13719 running    fn

% ps 13719
  PID TT  STAT      TIME COMMAND
13719 p0  T      0:00.00 zsh

That's why you had to kill -9 it. 
Now, what's this bit: "[1]  - 13719"...
                            ^
			    ^ ?????
% fg
fg: no current job
% %
fg: no such job: 2

:% fg %1
:^C^C^C^C^C^C^C^Z^Z^Z^Z^Z^Z^Z^Z

Regards,
-- 
Geoff Wing : <gcw@pobox.com>     Work URL: http://www.primenet.com.au/
Rxvt Stuff : <gcw@rxvt.org>      Ego URL : http://pobox.com/~gcw/
Zsh Stuff  : <gcw@zsh.org>       Phone   : (Australia) 0413 431 874

Re: Job control bug with pws-25

[Sven Wischnowsky]

Peter Stephenson wrote:

> Sven Wischnowsky wrote:
> > You didn't miss a patch -- I forgot to play with `bg'. Whew, this was
> > a bit more complicated. So much so, that I finally put some of the
> > things distributed in jobs.c into functions (super_job() and
> > handle_sub()).
> 
> Much improved, thanks.  There's still a small bug:
> 
> % fn() { sleep2; print foo; }
> % fn
> ^Z
> % bg
> % foo
> 
> The function exited, but no message was printed; notify is set.  Running
> `jobs' or exiting the shell prints the message.  Runing fn directly in the
> background behaves as expected.  (I think we had something like this once
> before, but I don't think it was there when the current saga started.)

I think the patch below is the right fix. It says roughly that we are
done setting up the super-job once we have turned it into one.

There is still a small problem: sometimes I get an empty line before a 
job-status message even when I shouldn't (because it isn't needed). I
haven't found out why the code sometimes thinks it should print that
when it shouldn't (but I consider this problem as being not too
important).

> It seems to me it might be a good idea to collect some of the things which
> can potentially go wrong with job control (as well as some which shouldn't
> :-)) and put them in a file, say, Misc/job-control-tests.  It doesn't need
> to run the tests itself, that's far too hard when the bugs are largely
> interactive, just give a collection of code with comments saying what you
> should do to see if it's working.  If I get a moment I'll have a look back
> in the archive for some.

Yes, and I'll probably have to put some more comments everywhere.
Unfortunately I'll be very busy this week again.

Bye
 Sven

diff -u os/exec.c Src/exec.c
--- os/exec.c	Mon Jul  5 12:00:24 1999
+++ Src/exec.c	Mon Jul  5 12:58:50 1999
@@ -916,7 +916,7 @@
 		    }
 
 		    jn->stat &= ~(STAT_DONE | STAT_NOPRINT);
-		    jn->stat |= STAT_STOPPED | STAT_CHANGED;
+		    jn->stat |= STAT_STOPPED | STAT_CHANGED | STAT_LOCKED;
 		    printjob(jn, !!isset(LONGLISTJOBS), 1);
 		}
 		else if (newjob != list_pipe_job)

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Job control bug with pws-25

[Sven Wischnowsky]

Peter Stephenson wrote:

> Actually, as soon as I had I chance to look I found some more job control
> bugs (hope I got all the right patches into pws-25).  If you do
> 
> while true; do zcat; done
> 
> and hit ^C repeatedly, the zcat gets killed but the loop continues.
> Eventually, however, you catch the shell when it's not running zcat and the
> loop exits.  Doing this a few times gives the message
> 
> zsh: job table full
> 
> and you can't run any more external jobs, so it's not getting deleted
> properly.

Right. The test in exec.c was too restrictive.

> Secondly (and this really makes me suspect I've missed a patch):
> 
> % fn() { sleep 2; print foo; }
> % fn
> ^Zzsh: 13719 suspended  fn
> % bg
> [1]  - continued  fn
> <after a while>
> % jobs -l
> [1]  - 13719 running    fn
> % fg %1
> ^C^C^C^C^C^C^C^Z^Z^Z^Z^Z^Z^Z^Z
> 
> no effect.  Shell has to killed with -9 from somewhere else.  Note that it
> has forked, as expected, and doesn't seem to be doing anything.

You didn't miss a patch -- I forgot to play with `bg'. Whew, this was
a bit more complicated. So much so, that I finally put some of the
things distributed in jobs.c into functions (super_job() and
handle_sub()).
The problem was that we had code to wake up the sub-shell stored in
the super-job only in waitjob(), i.e. for foreground jobs. But with
`bg' the sub-job was continued and when it finished nobody SIGCONT'ed
the sub-shell of the super-job and that's where it hung. So I've put
that code into handle_sub() and made it be called from update_job().
Then there was a bit of fiddling for a case as your example when you
bg the job and immediatly fg it again (before the sleep finished). In
such a case we have to attach the tty to the sub-shell immediatly.


mason@primenet.com.au wrote:

> Now, what's this bit: "[1]  - 13719"...
>                             ^
> 			    ^ ?????

setprevjob() didn't know about sub-jobs and messed up our
prevjob/curjob settings. This should be fixed, too.

Thanks for your help. And: sorry!

Bye
 Sven

P.S.: Due to the putting-in-functions this looks rather biggish.

diff -u os/exec.c Src/exec.c
--- os/exec.c	Mon Jul  5 10:24:20 1999
+++ Src/exec.c	Mon Jul  5 10:40:05 1999
@@ -1012,7 +1012,9 @@
 		jn = jobtab + pj;
 		killjb(jn, lastval & ~0200);
 	    }
-	    if (list_pipe_child || (list_pipe && (jn->stat & STAT_DONE)))
+	    if (list_pipe_child ||
+		((jn->stat & STAT_DONE) &&
+		 (list_pipe || (pline_level && !(jn->stat & STAT_SUBJOB)))))
 		deletejob(jn);
 	    thisjob = pj;
 
diff -u os/jobs.c Src/jobs.c
--- os/jobs.c	Mon Jul  5 10:24:21 1999
+++ Src/jobs.c	Mon Jul  5 11:50:46 1999
@@ -125,6 +125,86 @@
     return 0;
 }
 
+/* Find the super-job of a sub-job. */
+
+/**/
+static int
+super_job(int sub)
+{
+    int i;
+
+    for (i = 1; i < MAXJOB; i++)
+	if ((jobtab[i].stat & STAT_SUPERJOB) &&
+	    jobtab[i].other == sub &&
+	    jobtab[i].gleader)
+	    return i;
+    return 0;
+}
+
+/**/
+static int
+handle_sub(int job, int fg)
+{
+    Job jn = jobtab + job, sj = jobtab + jn->other;
+
+    if ((sj->stat & STAT_DONE) || !sj->procs) {
+	struct process *p;
+		    
+	for (p = sj->procs; p; p = p->next)
+	    if (WIFSIGNALED(p->status)) {
+		if (jn->gleader != mypgrp && jn->procs->next)
+		    killpg(jn->gleader, WTERMSIG(p->status));
+		else
+		    kill(jn->procs->pid, WTERMSIG(p->status));
+		kill(sj->other, SIGCONT);
+		kill(sj->other, WTERMSIG(p->status));
+		break;
+	    }
+	if (!p) {
+	    int cp;
+
+	    jn->stat &= ~STAT_SUPERJOB;
+	    jn->stat |= STAT_WASSUPER;
+
+	    if ((cp = ((WIFEXITED(jn->procs->status) ||
+			WIFSIGNALED(jn->procs->status)) &&
+		       killpg(jn->gleader, 0) == -1))) {
+		Process p;
+		for (p = jn->procs; p->next; p = p->next);
+		jn->gleader = p->pid;
+	    }
+	    /* This deleted the job too early if the parent
+	       shell waited for a command in a list that will
+	       be executed by the sub-shell (e.g.: if we have
+	       `ls|if true;then sleep 20;cat;fi' and ^Z the
+	       sleep, the rest will be executed by a sub-shell,
+	       but the parent shell gets notified for the
+	       sleep.
+	       deletejob(sj); */
+	    /* If this super-job contains only the sub-shell,
+	       we have to attach the tty to its process group
+	       now. */
+	    if ((fg || thisjob == job) &&
+		(!jn->procs->next || cp || jn->procs->pid != jn->gleader))
+		attachtty(jn->gleader);
+	    kill(sj->other, SIGCONT);
+	}
+	curjob = jn - jobtab;
+    } else if (sj->stat & STAT_STOPPED) {
+	struct process *p;
+
+	jn->stat |= STAT_STOPPED;
+	for (p = jn->procs; p; p = p->next)
+	    if (p->status == SP_RUNNING ||
+		(!WIFEXITED(p->status) && !WIFSIGNALED(p->status)))
+		p->status = sj->procs->status;
+	curjob = jn - jobtab;
+	printjob(jn, !!isset(LONGLISTJOBS), 1);
+	return 1;
+    }
+    return 0;
+}
+
 /* Update status of process that we have just WAIT'ed for */
 
 /**/
@@ -183,13 +263,8 @@
 		 * or to exit. So we have to send it a SIGTSTP. */
 		int i;
 
-		for (i = 1; i < MAXJOB; i++)
-		    if ((jobtab[i].stat & STAT_SUPERJOB) &&
-			jobtab[i].other == job &&
-			jobtab[i].gleader) {
-			killpg(jobtab[i].gleader, SIGTSTP);
-			break;
-		    }
+		if ((i = super_job(job)))
+		    killpg(jobtab[i].gleader, SIGTSTP);
 	    }
 	    return;
 	}
@@ -254,6 +329,13 @@
 	return;
     jn->stat |= (somestopped) ? STAT_CHANGED | STAT_STOPPED :
 	STAT_CHANGED | STAT_DONE;
+    if (!inforeground &&
+	(jn->stat & (STAT_SUBJOB | STAT_DONE)) == (STAT_SUBJOB | STAT_DONE)) {
+	int su;
+
+	if ((su = super_job(jn - jobtab)))
+	    handle_sub(su, 0);
+    }
     if ((jn->stat & (STAT_DONE | STAT_STOPPED)) == STAT_STOPPED) {
 	prevjob = curjob;
 	curjob = job;
@@ -303,13 +385,14 @@
 
     for (i = MAXJOB - 1; i; i--)
 	if ((jobtab[i].stat & STAT_INUSE) && (jobtab[i].stat & STAT_STOPPED) &&
-	    i != curjob && i != thisjob) {
+	    !(jobtab[i].stat & STAT_SUBJOB) && i != curjob && i != thisjob) {
 	    prevjob = i;
 	    return;
 	}
 
     for (i = MAXJOB - 1; i; i--)
-	if ((jobtab[i].stat & STAT_INUSE) && i != curjob && i != thisjob) {
+	if ((jobtab[i].stat & STAT_INUSE) && !(jobtab[i].stat & STAT_SUBJOB) &&
+	    i != curjob && i != thisjob) {
 	    prevjob = i;
 	    return;
 	}
@@ -791,64 +874,9 @@
 		killjb(jn, SIGCONT);
 		jn->stat &= ~STAT_STOPPED;
 	    }
-	    if (jn->stat & STAT_SUPERJOB) {
-		Job sj = jobtab + jn->other;
-		if ((sj->stat & STAT_DONE) || !sj->procs) {
-		    struct process *p;
-		    
-		    for (p = sj->procs; p; p = p->next)
-			if (WIFSIGNALED(p->status)) {
-			    if (jn->gleader != mypgrp && jn->procs->next)
-				killpg(jn->gleader, WTERMSIG(p->status));
-			    else
-				kill(jn->procs->pid, WTERMSIG(p->status));
-			    kill(sj->other, SIGCONT);
-			    kill(sj->other, WTERMSIG(p->status));
-			    break;
-			}
-		    if (!p) {
-			int cp;
-
-			jn->stat &= ~STAT_SUPERJOB;
-			jn->stat |= STAT_WASSUPER;
-
-			if ((cp = ((WIFEXITED(jn->procs->status) ||
-				    WIFSIGNALED(jn->procs->status)) &&
-				   killpg(jn->gleader, 0) == -1))) {
-			    Process p;
-			    for (p = jn->procs; p->next; p = p->next);
-			    jn->gleader = p->pid;
-			}
-			/* This deleted the job too early if the parent
-			   shell waited for a command in a list that will
-			   be executed by the sub-shell (e.g.: if we have
-			   `ls|if true;then sleep 20;cat;fi' and ^Z the
-			   sleep, the rest will be executed by a sub-shell,
-			   but the parent shell gets notified for the
-			   sleep.
-			   deletejob(sj); */
-			/* If this super-job contains only the sub-shell,
-			   we have to attach the tty to our process group
-			   (which is shared by the sub-shell) now. */
-			if (!jn->procs->next || cp || jn->procs->pid != jn->gleader)
-			    attachtty(jn->gleader);
-			kill(sj->other, SIGCONT);
-		    }
-		    curjob = jn - jobtab;
-		}
-		else if (sj->stat & STAT_STOPPED) {
-		    struct process *p;
-
-		    jn->stat |= STAT_STOPPED;
-		    for (p = jn->procs; p; p = p->next)
-			if (p->status == SP_RUNNING ||
-			    (!WIFEXITED(p->status) && !WIFSIGNALED(p->status)))
-			    p->status = sj->procs->status;
-		    curjob = jn - jobtab;
-		    printjob(jn, !!isset(LONGLISTJOBS), 1);
+	    if (jn->stat & STAT_SUPERJOB)
+		if (handle_sub(jn - jobtab, 1))
 		    break;
-		}
-	    }
 	    child_block();
 	}
     } else

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Job control bug with pws-25

[Peter Stephenson]

Sven Wischnowsky wrote:
> You didn't miss a patch -- I forgot to play with `bg'. Whew, this was
> a bit more complicated. So much so, that I finally put some of the
> things distributed in jobs.c into functions (super_job() and
> handle_sub()).

Much improved, thanks.  There's still a small bug:

% fn() { sleep2; print foo; }
% fn
^Z
% bg
% foo

The function exited, but no message was printed; notify is set.  Running
`jobs' or exiting the shell prints the message.  Runing fn directly in the
background behaves as expected.  (I think we had something like this once
before, but I don't think it was there when the current saga started.)

It seems to me it might be a good idea to collect some of the things which
can potentially go wrong with job control (as well as some which shouldn't
:-)) and put them in a file, say, Misc/job-control-tests.  It doesn't need
to run the tests itself, that's far too hard when the bugs are largely
interactive, just give a collection of code with comments saying what you
should do to see if it's working.  If I get a moment I'll have a look back
in the archive for some.

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy