Not quite out of the pipestatus woods yet ...

Not quite out of the pipestatus woods yet ...

[Bart Schaefer]

If I add

setopt MONITOR

before the "stress test" in A05execution.ztst, it ends up failing most of
the time.  So there is indeed still a race of some sort.

-- 
Barton E. Schaefer

Re: Not quite out of the pipestatus woods yet ...

[Bart Schaefer]

On Oct 25,  4:25pm, Bart Schaefer wrote:
}
} setopt MONITOR
} 
} before the "stress test" in A05execution.ztst, it ends up failing most of
} the time.  So there is indeed still a race of some sort.

Some experimentation with "set -x" and TRAPCHLD indicates that it gets
pipestatus wrong if the left-hand-sides of the pipe exit while the
loop body is still running.

Some debugging statements added to storepipestats() indicate that the
problem is that job != thisjob at the time the loop finally does exit.

I think it has something to do with the oldjobtab / jobtab switching, but
have not yet confirmed.

Re: Not quite out of the pipestatus woods yet ...

[Bart Schaefer]

On Oct 26, 12:45am, Bart Schaefer wrote:
}
} I think it has something to do with the oldjobtab / jobtab switching, but
} have not yet confirmed.

Yes, this definitely it.

[[ DO NOT APPLY THIS DIFF AS A PATCH.  Included for explanation only. ]]

diff --git a/Src/jobs.c b/Src/jobs.c
index c218743..d5e316f 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -945,6 +945,13 @@ printjob(Job jn, int lng, int synch)
 	job = jn - oldjobtab;
     else
 	job = jn - jobtab;
+    /*
+    DPUTS3(job < 0 || job > maxjob,
+	   "bogus job number, jn = %L, jobtab = %L, oldjobtab = %L",
+	   (long)jn, (long)jobtab, (long)oldjobtab);
+    */
+    if (job < 0 || job > maxjob)
+	job = jn - jobtab;
 
     if (jn->stat & STAT_NOPRINT) {
 	skip_print = 1;

With this code change, the stress test never prints the wrong $pipestatus,
regardless of the order of signals arriving.

HOWEVER, with the "setopt MONITOR" added before the stress test, I have
had the shell go into a fast busy in acquire_pgrp() -- the while loop
condition never becomes false but the shell never gets SIGT* either.

I think that's a separate problem, but:  PWS, do you remember enough
about why there is an assumption that oldjobtab never needs to be set
back to NULL once it has been created?  Or is this just a case of not
realizing that printjob() has multiple responsibilities, including the
reaping of old jobs?

Re: Not quite out of the pipestatus woods yet ...

[Peter Stephenson]

On Sat, 26 Oct 2013 09:57:46 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> HOWEVER, with the "setopt MONITOR" added before the stress test, I have
> had the shell go into a fast busy in acquire_pgrp() -- the while loop
> condition never becomes false but the shell never gets SIGT* either.

(Could perhaps be the same problem as the "i'm sorry, I can't do that
without a terminal, dave" race with a function at the end of the
pipeline?)
 
> I think that's a separate problem, but:  PWS, do you remember enough
> about why there is an assumption that oldjobtab never needs to be set
> back to NULL once it has been created?  Or is this just a case of not
> realizing that printjob() has multiple responsibilities, including the
> reaping of old jobs?

Orignally, I'm pretty sure (because for once I can remember doing it;
perhaps the coffee machine was broken) that oldjobtab was created to
allow you to look at the main shell's jobs from a subshell; they
wouldn't be updated any more, but users weren't too worried about the
races as they just wanted e.g. to be able to pipe the output of jobs in
a one-off fashion.  In that case, of course, there's no question of
doing anything with it; it remains the same ancient history of the
origignal shell until the subshell dies.  There was also no question of
job control in a subshell in those days --- that came significantly
later --- so copying it and clearing the main jobtab, rather than using
the latter in situ and assuming it was frozen, may just have been
caution.

If oldjobtab has suffered mission creep since then, there's a good
chance that any additional circumstance has been overlooked.

pws

Re: Not quite out of the pipestatus woods yet ...

[Bart Schaefer]

On Oct 26,  9:57am, Bart Schaefer wrote:
} Subject: Re: Not quite out of the pipestatus woods yet ...
}
} On Oct 26, 12:45am, Bart Schaefer wrote:
} }
} } I think it has something to do with the oldjobtab / jobtab switching, but
} } have not yet confirmed.
} 
} Yes, this [is] definitely it.
} 
} HOWEVER, with the "setopt MONITOR" added before the stress test, I have
} had the shell go into a fast busy in acquire_pgrp() -- the while loop
} condition never becomes false but the shell never gets SIGT* either.

OK, I still don't recall why oldjobtab is hanging around all the time,
but the following seems to cover everything.  I updated the stress
test to use three different exit values to be sure those values were
assigned to pipestats in the correct order.

There remains a question of what happens if (jn->stat & STAT_DONE) for
an entry in oldjobtab.  I'm not sure what side effects fiddling with
curjob and prevjob may have in that case.  Maybe it never happens.


diff --git a/Src/jobs.c b/Src/jobs.c
index c218743..336c5d4 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -941,10 +941,13 @@ printjob(Job jn, int lng, int synch)
     int doneprint = 0, skip_print = 0;
     FILE *fout = (synch == 2 || !shout) ? stdout : shout;
 
-    if (oldjobtab != NULL)
+    if (synch > 1 && oldjobtab != NULL)
 	job = jn - oldjobtab;
     else
 	job = jn - jobtab;
+    DPUTS3(job < 0 || job > (synch > 1 ? oldmaxjob : maxjob),
+	   "bogus job number, jn = %L, jobtab = %L, oldjobtab = %L",
+	   (long)jn, (long)jobtab, (long)oldjobtab);
 
     if (jn->stat & STAT_NOPRINT) {
 	skip_print = 1;
@@ -995,7 +998,8 @@ printjob(Job jn, int lng, int synch)
     if (skip_print) {
 	if (jn->stat & STAT_DONE) {
 	    /* This looks silly, but see update_job() */
-	    storepipestats(jn, job == thisjob, job == thisjob);
+	    if (synch <= 1)
+		storepipestats(jn, job == thisjob, job == thisjob);
 	    if (should_report_time(jn))
 		dumptime(jn);
 	    deletejob(jn, 0);
@@ -1128,7 +1132,8 @@ printjob(Job jn, int lng, int synch)
 
     if (jn->stat & STAT_DONE) {
 	/* This looks silly, but see update_job() */
-	storepipestats(jn, job == thisjob, job == thisjob);
+	if (synch <= 1)
+	    storepipestats(jn, job == thisjob, job == thisjob);
 	if (should_report_time(jn))
 	    dumptime(jn);
 	deletejob(jn, 0);
@@ -2610,6 +2615,8 @@ acquire_pgrp(void)
 	while ((ttpgrp = gettygrp()) != -1 && ttpgrp != mypgrp) {
 	    mypgrp = GETPGRP();
 	    if (mypgrp == mypid) {
+		if (!interact)
+		    break; /* attachtty() will be a no-op, give up */
 		signal_setmask(oldset);
 		attachtty(mypgrp); /* Might generate SIGT* */
 		signal_block(blockset);
diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst
index 8578016..ba7e02c 100644
--- a/Test/A05execution.ztst
+++ b/Test/A05execution.ztst
@@ -179,17 +179,23 @@
 0:Status reset by starting a backgrounded command
 >0
 
-  repeat 2048; do (: | : | while false; do
+  setopt MONITOR
+  [[ -o MONITOR ]] || print -u $ZTST_fd 'Unable to change MONITOR option'
+  repeat 2048; do (return 2 |
+                   return 1 |
+                   while true; do
+                             false
                              break
                            done;
                    print "${pipestatus[@]}")
 	ZTST_hashmark
   done | sort | uniq -c | sed 's/^ *//'
 0:Check whether `$pipestatus[]' behaves.
->2048 0 0 0
+>2048 2 1 0
 F:This test checks for a bug in `$pipestatus[]' handling.  If it breaks then
 F:the bug is still there or it reappeared. See workers-29973 for details.
 
+  setopt MONITOR
   externFunc() { awk >/dev/null 2>&1; true; }
   false | true | false | true | externFunc
   echo $pipestatus