Re: PATCH: job-control

Re: PATCH: job-control

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> So now you're saying it works to do the child_unblock() *after* the
> waitjobs() rather than before?  That makes no sense; waitjobs() does
> a child_unblock() at the end.  So if the previous child_unblock()
> (inside the !STAT_LOCKED test) worked, then waitjobs() was getting
> called, and this child_unblock() shouldn't make any difference.

See 9498: it caused problems in a case where jobtab[thisjob] didn't
have any procs so that waitjobs() didn't call waitjob() -- and hence
no child_whatever().

> Unless this one is happening in a different call to execpline()?  Does
> the patch below work too?

No, because in that case we *were* calling waitjobs(), without any
effect, though.

> In any case, that last child_block() above should be outside the close
> brace.

Sorry, I had forgotten about that. Although it may be cleaner to put
it directly after the waitjobs() (and call it only if that was
called).

We could also enhance the test for the child{un,}block() to not call
them if we are sure that the waitjob() was called.

This patch instead of Bart's.

Bye
 Sven

diff -ru ../z.old/Src/exec.c Src/exec.c
--- ../z.old/Src/exec.c	Tue Feb  1 14:47:35 2000
+++ Src/exec.c	Wed Feb  2 09:20:29 2000
@@ -932,6 +932,7 @@
     } else {
 	if (newjob != lastwj) {
 	    Job jn = jobtab + newjob;
+	    int updated;
 
 	    if (newjob == list_pipe_job && list_pipe_child)
 		_exit(0);
@@ -980,10 +981,14 @@
 		    jn->stat |= STAT_NOPRINT;
 		    makerunning(jn);
 		}
-		if (!(jn->stat & STAT_LOCKED))
+		if (!(jn->stat & STAT_LOCKED)) {
+		    updated = !!jobtab[thisjob].procs;
 		    waitjobs();
-
-		if (list_pipe_job && jobtab[list_pipe_job].procs &&
+		    child_block();
+		} else
+		    updated = 0;
+		if (!updated &&
+		    list_pipe_job && jobtab[list_pipe_job].procs &&
 		    !(jobtab[list_pipe_job].stat & STAT_STOPPED)) {
 		    child_unblock();
 		    child_block();

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

Re: PATCH: job-control

[Bart Schaefer]

On Feb 1, 11:58am, Sven Wischnowsky wrote:
} Subject: Re: PATCH: job-control
}
} 
} I wrote:
} 
} > But removing those two child_* and
} > adding:
} > 
} >   if (list_pipe_job && jobtab[list_pipe_job].procs &&
} >       !(jobtab[list_pipe_job].stat & STAT_STOPPED))
} >       child_suspend(0);
} > 
} > before the if (!list_pipe_child && ...) fixes the problem, too and is
} > almost certainly better. Can anyone see a problem with this?
} 
} No, we would also need a child_block, then, so we can use
} child_unblock()/child_block() after this test.

Waitaminute, now I'm *really* confused.

} -		if (!(jn->stat & STAT_LOCKED)) {
} -		    child_unblock();
} +		if (!(jn->stat & STAT_LOCKED))
}  		    waitjobs();
} +
} +		if (list_pipe_job && jobtab[list_pipe_job].procs &&
} +		    !(jobtab[list_pipe_job].stat & STAT_STOPPED)) {
} +		    child_unblock();
}  		    child_block();
}  		}

So now you're saying it works to do the child_unblock() *after* the
waitjobs() rather than before?  That makes no sense; waitjobs() does
a child_unblock() at the end.  So if the previous child_unblock()
(inside the !STAT_LOCKED test) worked, then waitjobs() was getting
called, and this child_unblock() shouldn't make any difference.

Unless this one is happening in a different call to execpline()?  Does
the patch below work too?

In any case, that last child_block() above should be outside the close
brace.

Index: Src/exec.c
===================================================================
@@ -981,13 +981,11 @@
 		    makerunning(jn);
 		}
 		if (!(jn->stat & STAT_LOCKED))
-		    waitjobs();
-
-		if (list_pipe_job && jobtab[list_pipe_job].procs &&
-		    !(jobtab[list_pipe_job].stat & STAT_STOPPED)) {
-		    child_unblock();
-		    child_block();
-		}
+		    waitjobs();      /* Implicit child_unblock() */
+		else if (list_pipe_job && jobtab[list_pipe_job].procs &&
+		    !(jobtab[list_pipe_job].stat & STAT_STOPPED))
+		    child_unblock(); /* Permit job table update */
+		child_block();       /* Freeze job table again */
 		if (list_pipe_child &&
 		    jn->stat & STAT_DONE &&
 		    lastval2 & 0200)

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: job-control

[Sven Wischnowsky]

I wrote:

> But removing those two child_* and
> adding:
> 
>   if (list_pipe_job && jobtab[list_pipe_job].procs &&
>       !(jobtab[list_pipe_job].stat & STAT_STOPPED))
>       child_suspend(0);
> 
> before the if (!list_pipe_child && ...) fixes the problem, too and is
> almost certainly better. Can anyone see a problem with this?

No, we would also need a child_block, then, so we can use
child_unblock()/child_block() after this test.

I haven't found an example where this fails...

Bye
 Sven

diff -ru ../z.old/Src/exec.c Src/exec.c
--- ../z.old/Src/exec.c	Tue Feb  1 11:33:24 2000
+++ Src/exec.c	Tue Feb  1 11:34:43 2000
@@ -980,9 +980,12 @@
 		    jn->stat |= STAT_NOPRINT;
 		    makerunning(jn);
 		}
-		if (!(jn->stat & STAT_LOCKED)) {
-		    child_unblock();
+		if (!(jn->stat & STAT_LOCKED))
 		    waitjobs();
+
+		if (list_pipe_job && jobtab[list_pipe_job].procs &&
+		    !(jobtab[list_pipe_job].stat & STAT_STOPPED)) {
+		    child_unblock();
 		    child_block();
 		}
 		if (list_pipe_child &&

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

Re: PATCH: job-control

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Jan 31, 11:00am, Sven Wischnowsky wrote:
> } Subject: Re: PATCH: job-control
> }
> } Bart Schaefer wrote:
> } 
> } > If it really is somehow the case the "it found out that the pipe-leader
> } > was suspended too late," then it seems to me that the while() condition
> } > in waitjob() is what needs fixing, or we still have a race condition:
> } > the ^Z could suspend the pipe-leader between the child_block() and the
> } > while() test within waitjob().  All that this change has done is shrink
> } > the window.
> } 
> } No, the important bit is the child_unblock() which makes the signal
> } handler be run for all pending signals (we are blocking child signals
> } during most of the execution code), so that the job and process
> } infos are updated.
> 
> Yes, that's exactly my point.  waitjob() should enter the body of the
> while() loop -- thus calling child_suspend() and allowing the job and
> process info to be updated -- when there are any jobs that the shell
> "believes" are still in a runnable state.  It should never be the case
> that the job info has to be updated by a signal handler in order for
> the shell to discover that there may be runnable jobs; in that case it
> can mean only that (a) the setup of the info for those jobs is wrong
> to begin with, or (b) there's a condition in which the loop should be 
> entered but that is not tested.
> 
> The other possibility is that child_suspend() isn't sufficient to get
> the job info updated, but that would imply a much more serious problem.
> 
> } Without the patch this happened only when a
> } execpline() finished (shortly before that). In the test case there
> } were two of them active and we need to know that the leader was
> } suspended in the inner one but since child-signals were only delivered 
> } after the call to waitjobs(), we could see that only in the outer
> } execpline().
> 
> When you say "we need to know that the leader was suspended in the
> inner one," what does that mean code-wise?  What is it that we "see
> only in the outer execpline()"?

Ok, I've done some testing with the child_unblock() commented out
again and now I remember...

As a reminder, the problem was with this invocation:

  foo() {
    local a
    while read a; do :; done
    less "$@"
  }
  cat builtin.c | f glob.c

and hitting ^Z while the cat is still running.

In this case, when we hit the call to waitjobs() for the first time,
the job tested below it (the one for the loop) has no processes and
hence we don't even enter waitjob(). But some lines below the call to
waitjobs() we need the updated job-entry for the list_pipe_job.

Hm, efore I tried again, I was about to suggest to do the signal
magic only if there are any processes because it's rather expensive,
but obviously that would be wrong. But removing those two child_* and
adding:

  if (list_pipe_job && jobtab[list_pipe_job].procs &&
      !(jobtab[list_pipe_job].stat & STAT_STOPPED))
      child_suspend(0);

before the if (!list_pipe_child && ...) fixes the problem, too and is
almost certainly better. Can anyone see a problem with this?

Bye
 Sven


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

Re: PATCH: job-control

[Bart Schaefer]

On Jan 31, 11:00am, Sven Wischnowsky wrote:
} Subject: Re: PATCH: job-control
}
} > Also, the child_block() call following the unblock in exec.c is redundant,
} > because block/unblock are not stacked and the first thing that waitjobs()
} > does [via waitjob()] is to call child_block().
} 
} Yes, I saw that at the weekend, too (I'm playing with a non-recursive
} execution code at home).
} 
}  		if (!(jn->stat & STAT_LOCKED)) {
}  		    child_unblock();
} -		    child_block();
}  		    waitjobs();
}  		}

Actually, I just realized that that's not right either.  It's not safe to
go on examining jn->stat and jobtab[list_pipe_job] etc. in the rest of
execpline() with child_unblock() in effect (it's left unblocked when
waitjobs() returns).

So we may end up removing the child_unblock() there, but I think we need
to block again after waitjobs().

Index: Src/exec.c
===================================================================
@@ -983,6 +983,7 @@
 		if (!(jn->stat & STAT_LOCKED)) {
 		    child_unblock();
 		    waitjobs();
+		    child_block();
 		}
 		if (list_pipe_child &&
 		    jn->stat & STAT_DONE &&

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: job-control

[Bart Schaefer]

On Jan 31, 11:00am, Sven Wischnowsky wrote:
} Subject: Re: PATCH: job-control
}
} Bart Schaefer wrote:
} 
} > If it really is somehow the case the "it found out that the pipe-leader
} > was suspended too late," then it seems to me that the while() condition
} > in waitjob() is what needs fixing, or we still have a race condition:
} > the ^Z could suspend the pipe-leader between the child_block() and the
} > while() test within waitjob().  All that this change has done is shrink
} > the window.
} 
} No, the important bit is the child_unblock() which makes the signal
} handler be run for all pending signals (we are blocking child signals
} during most of the execution code), so that the job and process
} infos are updated.

Yes, that's exactly my point.  waitjob() should enter the body of the
while() loop -- thus calling child_suspend() and allowing the job and
process info to be updated -- when there are any jobs that the shell
"believes" are still in a runnable state.  It should never be the case
that the job info has to be updated by a signal handler in order for
the shell to discover that there may be runnable jobs; in that case it
can mean only that (a) the setup of the info for those jobs is wrong
to begin with, or (b) there's a condition in which the loop should be 
entered but that is not tested.

The other possibility is that child_suspend() isn't sufficient to get
the job info updated, but that would imply a much more serious problem.

} Without the patch this happened only when a
} execpline() finished (shortly before that). In the test case there
} were two of them active and we need to know that the leader was
} suspended in the inner one but since child-signals were only delivered 
} after the call to waitjobs(), we could see that only in the outer
} execpline().

When you say "we need to know that the leader was suspended in the
inner one," what does that mean code-wise?  What is it that we "see
only in the outer execpline()"?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: job-control

[Sven Wischnowsky]

Bart Schaefer wrote:

> (Yes, I'm still behind.)
> 
> On Jan 18, 12:36pm, Sven Wischnowsky wrote:
> } Subject: PATCH: job-control
> }
> } Here is the fix for the problem mentioned in 9332.
> } 
> } The second problem turned out to be two problems. If one hit ^Z early
> } enough to suspend the cat, the then-sub-shelled loop wasn't continued
> } and the less wasn't executed.  [...] This was because at the time of
> } the inner waitjobs() child_block() was active so it found out that the
> } pipe-leader was suspended too late. This is fixed by the hunk that
> } adds a child_block/unblock() before the waitjobs(). This is the hunk
> } I'm least happy about. Bart, can you think of any problems with this?
> 
> The only "problem" is that I can't figure out why it makes any difference.
> 
> If it really is somehow the case the "it found out that the pipe-leader
> was suspended too late," then it seems to me that the while() condition
> in waitjob() is what needs fixing, or we still have a race condition:
> the ^Z could suspend the pipe-leader between the child_block() and the
> while() test within waitjob().  All that this change has done is shrink
> the window.

No, the important bit is the child_unblock() which makes the signal
handler be run for all pending signals (we are blocking child signals
during most of the execution code), so that the job and process
infos are updated. Without the patch this happened only when a
execpline() finished (shortly before that). In the test case there
were two of them active and we need to know that the leader was
suspended in the inner one but since child-signals were only delivered 
after the call to waitjobs(), we could see that only in the outer
execpline().

> Also, the child_block() call following the unblock in exec.c is redundant,
> because block/unblock are not stacked and the first thing that waitjobs()
> does [via waitjob()] is to call child_block().

Yes, I saw that at the weekend, too (I'm playing with a non-recursive
execution code at home).

Bye
 Sven

diff -ru ../z.old/Src/exec.c Src/exec.c
--- ../z.old/Src/exec.c	Mon Jan 31 10:49:45 2000
+++ Src/exec.c	Mon Jan 31 10:56:15 2000
@@ -982,7 +982,6 @@
 		}
 		if (!(jn->stat & STAT_LOCKED)) {
 		    child_unblock();
-		    child_block();
 		    waitjobs();
 		}
 		if (list_pipe_child &&

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

Re: PATCH: job-control

[Bart Schaefer]

(Yes, I'm still behind.)

On Jan 18, 12:36pm, Sven Wischnowsky wrote:
} Subject: PATCH: job-control
}
} Here is the fix for the problem mentioned in 9332.
} 
} The second problem turned out to be two problems. If one hit ^Z early
} enough to suspend the cat, the then-sub-shelled loop wasn't continued
} and the less wasn't executed.  [...] This was because at the time of
} the inner waitjobs() child_block() was active so it found out that the
} pipe-leader was suspended too late. This is fixed by the hunk that
} adds a child_block/unblock() before the waitjobs(). This is the hunk
} I'm least happy about. Bart, can you think of any problems with this?

The only "problem" is that I can't figure out why it makes any difference.

If it really is somehow the case the "it found out that the pipe-leader
was suspended too late," then it seems to me that the while() condition
in waitjob() is what needs fixing, or we still have a race condition:
the ^Z could suspend the pipe-leader between the child_block() and the
while() test within waitjob().  All that this change has done is shrink
the window.

Also, the child_block() call following the unblock in exec.c is redundant,
because block/unblock are not stacked and the first thing that waitjobs()
does [via waitjob()] is to call child_block().

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

PATCH: job-control

[Sven Wischnowsky]

Here is the fix for the problem mentioned in 9332. This:

  # Try
  #   ^Z
  #   fg
  #   ^Z
  #   fg
  fn() {
    local a
    while read a; do :; done
    less "$@"
  }
  cat foo | fn bar

didn't work.

The first problem was that at the time the sub-shell was created, the
cat wasn't running anymore. So the sub-shell was put in its own process
group and the gleader field for the job was set to that. Then bin_fg()
didn't realise that the gleader only referred to the sub-shell and
attached that to the terminal instead of the less, as it should
have. I've solved this by adding another STAT_* flag (STAT_SUBLEADER)
that gets set if a job's gleader is set to a sub-shell's pgrp because
the other commands in the pipe have finished already and bin_fg()
tests this flag to find out which pgrp to attach.

The second problem turned out to be two problems. If one hit ^Z early
enough to suspend the cat, the then-sub-shelled loop wasn't continued
and the less wasn't executed. First, the sub-shell had simply returned 
from too many functions, waiting in the execpline() for the whole loop 
instead one of those in the loop. This was because at the time of the
inner waitjobs() child_block() was active so it found out that the
pipe-leader was suspended too late. This is fixed by the hunk that
adds a child_block/unblock() before the waitjobs(). This is the hunk
I'm least happy about. Bart, can you think of any problems with this?
The second sub-problem was that update_job() set errflag and breaks to 
tell the parent shell to leave the loop. But since the sub-shell
inherits these, it left the loop, too, even though it should have
continued it. So I made them be restored to their previous values in 
the sub-shell.

Bye
 Sven

diff -ru ../z.old/Src/exec.c Src/exec.c
--- ../z.old/Src/exec.c	Tue Jan 18 11:04:15 2000
+++ Src/exec.c	Tue Jan 18 12:20:16 2000
@@ -949,9 +949,10 @@
 
 		    /* If the super-job contains only the sub-shell, the
 		       sub-shell is the group leader. */
-		    if (!jn->procs->next || lpforked == 2)
+		    if (!jn->procs->next || lpforked == 2) {
 			jn->gleader = list_pipe_pid;
-
+			jn->stat |= STAT_SUBLEADER;
+		    }
 		    for (pn = jobtab[jn->other].procs; pn; pn = pn->next)
 			if (WIFSTOPPED(pn->status))
 			    break;
@@ -971,14 +972,17 @@
 		    lastwj = -1;
 	    }
 
+	    errbrk_saved = 0;
 	    for (; !nowait;) {
 		if (list_pipe_child) {
 		    jn->stat |= STAT_NOPRINT;
 		    makerunning(jn);
 		}
-		if (!(jn->stat & STAT_LOCKED))
+		if (!(jn->stat & STAT_LOCKED)) {
+		    child_unblock();
+		    child_block();
 		    waitjobs();
-
+		}
 		if (list_pipe_child &&
 		    jn->stat & STAT_DONE &&
 		    lastval2 & 0200)
@@ -1042,6 +1046,10 @@
 			list_pipe = 0;
 			list_pipe_child = 1;
 			opts[INTERACTIVE] = 0;
+			if (errbrk_saved) {
+			    errflag = prev_errflag;
+			    breaks = prev_breaks;
+			}
 			break;
 		    }
 		}
diff -ru ../z.old/Src/jobs.c Src/jobs.c
--- ../z.old/Src/jobs.c	Tue Jan 18 11:04:16 2000
+++ Src/jobs.c	Tue Jan 18 11:53:47 2000
@@ -64,7 +64,13 @@
  
 /**/
 int ttyfrozen;
- 
+
+/* Previous values of errflag and breaks if the signal handler had to
+ * change them. And a flag saying if it did that. */
+
+/**/
+int prev_errflag, prev_breaks, errbrk_saved;
+
 static struct timeval dtimeval, now;
 
 /* Diff two timevals for elapsed-time computations */
@@ -311,6 +317,11 @@
 		/* If we have `foo|while true; (( x++ )); done', and hit
 		 * ^C, we have to stop the loop, too. */
 		if ((val & 0200) && inforeground == 1) {
+		    if (!errbrk_saved) {
+			errbrk_saved = 1;
+			prev_breaks = breaks;
+			prev_errflag = errflag;
+		    }
 		    breaks = loops;
 		    errflag = 1;
 		    inerrflush();
@@ -322,6 +333,11 @@
 	    }
 	}
     } else if (list_pipe && (val & 0200) && inforeground == 1) {
+	if (!errbrk_saved) {
+	    errbrk_saved = 1;
+	    prev_breaks = breaks;
+	    prev_errflag = errflag;
+	}
 	breaks = loops;
 	errflag = 1;
 	inerrflush();
@@ -1296,6 +1312,7 @@
 		    thisjob = job;
 		    if ((jobtab[job].stat & STAT_SUPERJOB) &&
 			((!jobtab[job].procs->next ||
+			  (jobtab[job].stat & STAT_SUBLEADER) ||
 			  killpg(jobtab[job].gleader, 0) == -1)) &&
 			jobtab[jobtab[job].other].gleader)
 			attachtty(jobtab[jobtab[job].other].gleader);
diff -ru ../z.old/Src/zsh.h Src/zsh.h
--- ../z.old/Src/zsh.h	Tue Jan 18 11:04:19 2000
+++ Src/zsh.h	Tue Jan 18 11:10:00 2000
@@ -636,6 +636,7 @@
 #define STAT_NOSTTY	(1<<11)	/* the tty settings are not inherited   */
 				/* from this job when it exits.         */
 #define STAT_ATTACH	(1<<12)	/* delay reattaching shell to tty       */
+#define STAT_SUBLEADER  (1<<13) /* is super-job, but leader is sub-shell */
 
 #define SP_RUNNING -1		/* fake status for jobs currently running */
 

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