Re: Strange function/pipestatus behavior, maybe a scope bug?

[Bart Schaefer <schaefer@brasslantern.com>]

On Oct 24,  9:57am, Peter Stephenson wrote:
} Subject: Re: Strange function/pipestatus behavior, maybe a scope bug?
}
} On Wed, 23 Oct 2013 22:15:48 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > something goes wrong and the whole thing is treated as if it is in
} > the background.  Here's zsh-5.0.2-248-ge05261f without my patch:
} > 
} > % true | false | true | false | (){ true }; echo $pipestatus
} > [1]  + done       true | 
} >        exit 1     false | 
} >        done       true | 
} >        running    false
} > 1
} > % 
} 
} Sounds like a race setting up the job, maybe related to the fact that
} "thisjob" isn't initialised in a particularly obvious place when the
} code is executing within the shell.  Are you seeing this with any
} shell function, or just anonymous functions?

Any shell function will do it, except one that runs an external command.

The odd thing is that this doesn't require "zsh -f", it happens for the
first interactive command in a new shell even after loading the whole
of compinit etc.

It does NOT happen if the MONITOR option is not set.

Here's a different clue:

% true | false | (){ sleep 10; true }
zsh: done       true | 
zsh: exit 1     false | 
zsh: suspended  () { ... }
% echo $? : $pipestatus
20 : 0 1 0
% fg
[1]  + done       true | 
       exit 1     false | 
       continued  () { ... }
(anon):fg: no job control in this shell.
% 

This one is repeatable (happens every time, not just the first, and
with any function, not just an anonymous one).
 
} > Oh, one other bug which I don't believe is new:  the PIPEFAIL option
} > doesn't work right if the last command is a builtin.  Maybe that
} > test should be moved into storepipestats() as well, in which case
} > the redundancy I mentioned above might also be eliminated.
} 
} Are you saying that $pipestatus is correct but the overall status is
} wrong if you have something like "true | true | true" or "true | true |
} false"?

Not exactly.  PIPEFAIL means that if any command anywhere in the pipeline
fails, then the entire pipeline fails.  *Without* PIPEFAIL, the command

	true | (return 3) | true

shoule have pipefail=(0 3 0) and exit status 0.   *With* PIPEFAIL, it
should have pipefail=(0 3 0) and exit status 3.  But it does not,
becuase "true" is a builtin.  Replace with /bin/true and the PIPEFAIL
exit status is correct.

} It certainly sounds like they ought to run at the same point.

Yeah, I'm becoming convinced that the PIPEFAIL tests also need to be
moved into storepipestats().

It's a little messy that you can't tell from the Job pointer whether
the job *number* is equal to thisjob.  Since inforground can be true
in update_job even when job != thisjob, but PIPEFAIL only applies
when job == thisjob, it probably means passing an extra parameter.

Having implemented this, I can't force a case where it's the right
thing to call storepipestats() from update_job().  On the other hand,
I'm also getting signal handling race conditions again, particularly
when sending TSTP (^Z) to a pipeline that ends with (){ sleep N } --
the value of pipestatus is correct, but the job gets treated as done
when it should be stopped, so the stopped sleep is orphaned.

Somebody else please try this patch and see if it works consistently
for you.

Aside:  I did think of another reason we might want to keep a copy of
pipestatus in the Job object:  so that we can restore it when a job
that has been stopped or backgrounded is brought into the foreground
again.  Right now the exit status of "fg" seems to always be zero
even after suspending/resuming (){ sleep 10; false }, and pipestatus
is only valid right after suspending the job, not upon resuming it.

diff --git a/Src/jobs.c b/Src/jobs.c
index 82ffdf2..c218743 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -377,8 +377,8 @@ check_cursh_sig(int sig)
 }
 
 /**/
-int
-storepipestats(Job jn, int inforeground)
+void
+storepipestats(Job jn, int inforeground, int fixlastval)
 {
     int i, pipefail = 0, jpipestats[MAX_PIPESTATS];
     Process p;
@@ -397,7 +397,13 @@ storepipestats(Job jn, int inforeground)
 	numpipestats = i;
     }
 
-    return pipefail;
+    if (fixlastval) {
+      if (jn->stat & STAT_CURSH) {
+	if (!lastval && isset(PIPEFAIL))
+	  lastval = pipefail;
+      } else if (isset(PIPEFAIL))
+	lastval = pipefail;
+    }
 }
 
 /* Update status of job, possibly printing it */
@@ -532,15 +538,12 @@ update_job(Job jn)
     jn->stat |= (somestopped) ? STAT_CHANGED | STAT_STOPPED :
 	STAT_CHANGED | STAT_DONE;
     if (jn->stat & STAT_DONE) {
-	int newlastval = storepipestats(jn, inforeground);
-
-	if (job == thisjob) {
-	    if (jn->stat & STAT_CURSH) {
-		if (!lastval && isset(PIPEFAIL))
-		    lastval = newlastval;
-	    } else if (isset(PIPEFAIL))
-		lastval = newlastval;
-	}
+	/* This may be redundant with printjob() but note that inforeground
+	 * is true here for STAT_CURSH jobs even when job != thisjob, most
+	 * likely because thisjob = -1 from exec.c:execsimple() trickery.
+	 * However, if we reset lastval here we break it for printjob().
+	 */
+	storepipestats(jn, inforeground, 0);
     }
     if (!inforeground &&
 	(jn->stat & (STAT_SUBJOB | STAT_DONE)) == (STAT_SUBJOB | STAT_DONE)) {
@@ -991,7 +994,8 @@ printjob(Job jn, int lng, int synch)
 
     if (skip_print) {
 	if (jn->stat & STAT_DONE) {
-	  (void) storepipestats(jn, job == thisjob);
+	    /* This looks silly, but see update_job() */
+	    storepipestats(jn, job == thisjob, job == thisjob);
 	    if (should_report_time(jn))
 		dumptime(jn);
 	    deletejob(jn, 0);
@@ -1107,9 +1111,9 @@ printjob(Job jn, int lng, int synch)
 	fflush(fout);
     }
 
-/* print "(pwd now: foo)" messages: with (lng & 4) we are printing
- * the directory where the job is running, otherwise the current directory
- */
+    /* print "(pwd now: foo)" messages: with (lng & 4) we are printing
+     * the directory where the job is running, otherwise the current directory
+     */
 
     if ((lng & 4) || (interact && job == thisjob &&
 		      jn->pwd && strcmp(jn->pwd, pwd))) {
@@ -1119,10 +1123,12 @@ printjob(Job jn, int lng, int synch)
 	fprintf(fout, ")\n");
 	fflush(fout);
     }
-/* delete job if done */
+
+    /* delete job if done */
 
     if (jn->stat & STAT_DONE) {
-	(void) storepipestats(jn, job == thisjob);
+	/* This looks silly, but see update_job() */
+	storepipestats(jn, job == thisjob, job == thisjob);
 	if (should_report_time(jn))
 	    dumptime(jn);
 	deletejob(jn, 0);