Re: Zsh: [7] + 23074 suspended (tty output)

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On Sun, 16 Sep 2018 17:40:48 +0200
TS <debts@xxxxxxx> wrote:
> I have now tried different iterations of that minimal .zshrc.
> It seems only when less is wrapped inside a function i get this
> "zsh: suspended (tty output)".

Thanks for that --- it does help to have all the reproduction detail inline
(or attached) in a post on the list.

> But when less is wrapped inside a function like this, it is rather reliable
> replicable.

All the previous forms of this bug show this when inside a sourced file, so e.g.
testfile containing

echo | { cat | more; }

followed by ". ./testfile" shows this up.

I've rebased job_control_debug2 on top of all the latest fixes to track
this down.  (Just the sort of thing I like doing in my hotel room on
holiday.)

The issue is that the list_pipe_job mechanism is being applied twice,
once for the "." and once for the nested code (a function in your case,
a nested pipeline in the above case).  Regular readers will know one
application is quite enough --- quite whether doing this twice is a
good idea is anybody's guess.  Anyway, the first job is marked as being
in control of the terminal; then the second job gets the terminal.  The
first job exits and the shell erroneously thinks it was still in
control of the terminal and swipes it back from under the feet of the
second.

We need to keep track of which job's process group last got the
terminal.  I think the combination of recording this in attachtty() and
also in the recently added code that fixes up the records in the main
shell from what the new subshell just did should do the trick --- we're
still protected by the STAT_NOSTTY check with the additional safety.
However, I'm not sure we want to rush this fix out to all and sundry.
The trouble is, clearly no one's testing things until they're released
(which is why the releases are broken).  If this is likely to come
unstuck anywhere, I'd suspect use of ^Z and fg which is applying
the most additional stress to the foreground terminal machinations,
so if you have things you do with ^Z and complex shell constructs
please try them out.

I've also tightened up in entersubsh() so that if the process is
asynchronous it doesn't pass up the status --- that's all the
exec.c hunks are doing.

diff --git a/Src/exec.c b/Src/exec.c
index b9af9ea63..a667b078d 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1036,7 +1036,7 @@ entersubsh(int flags, struct entersubsh_ret *retp)
 		if (!(flags & ESUB_ASYNC))
 		    attachtty(jobtab[thisjob].gleader);
 	    }
-	    if (retp) {
+	    if (retp && !(flags & ESUB_ASYNC)) {
 		retp->gleader = jobtab[list_pipe_job].gleader;
 		retp->list_pipe_job = list_pipe_job;
 	    }
@@ -1058,12 +1058,13 @@ entersubsh(int flags, struct entersubsh_ret *retp)
 		!jobtab[list_pipe_job].gleader)
 		jobtab[list_pipe_job].gleader = jobtab[thisjob].gleader;
 	    setpgrp(0L, jobtab[thisjob].gleader);
-	    if (!(flags & ESUB_ASYNC))
+	    if (!(flags & ESUB_ASYNC)) {
 		attachtty(jobtab[thisjob].gleader);
-	    if (retp) {
-		retp->gleader = jobtab[thisjob].gleader;
-		if (list_pipe_job != thisjob)
-		    retp->list_pipe_job = list_pipe_job;
+		if (retp) {
+		    retp->gleader = jobtab[thisjob].gleader;
+		    if (list_pipe_job != thisjob)
+			retp->list_pipe_job = list_pipe_job;
+		}
 	    }
 	}
     }
diff --git a/Src/jobs.c b/Src/jobs.c
index db2e87ec1..2d58319a8 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -40,6 +40,11 @@ mod_export pid_t origpgrp;
 
 /**/
 mod_export pid_t mypgrp;
+
+/* the last process group to attach to the terminal */
+
+/**/
+pid_t last_attached_pgrp;
  
 /* the job we are working on */
  
@@ -1405,6 +1410,11 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime,
 	    jobtab[thisjob].gleader = gleader;
 	    if (list_pipe_job_used != -1)
 		jobtab[list_pipe_job_used].gleader = gleader;
+	    /*
+	     * Record here this is the latest process group to grab the
+	     * terminal as attachtty() was run in the subshell.
+	     */
+	    last_attached_pgrp = gleader;
 	} else if (!jobtab[thisjob].gleader)
 		jobtab[thisjob].gleader = pid;
 	/* attach this process to end of process list of current job */
diff --git a/Src/signals.c b/Src/signals.c
index 99aad0fab..26d88abc2 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -540,8 +540,8 @@ wait_for_processes(void)
 		if (WIFEXITED(status) &&
 		    pn->pid == jn->gleader &&
 		    killpg(pn->pid, 0) == -1) {
-		    jn->gleader = 0;
-		    if (!(jn->stat & STAT_NOSTTY)) {
+		    if (last_attached_pgrp == jn->gleader &&
+			!(jn->stat & STAT_NOSTTY)) {
 			/*
 			 * This PID was in control of the terminal;
 			 * reclaim terminal now it has exited.
@@ -552,6 +552,7 @@ wait_for_processes(void)
 			attachtty(mypgrp);
 			adjustwinsize(0);
 		    }
+		    jn->gleader = 0;
 		}
 	    }
 	    update_job(jn);
diff --git a/Src/utils.c b/Src/utils.c
index 075d27241..5a9fbdd32 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -4670,6 +4670,10 @@ attachtty(pid_t pgrp)
 		ep = 1;
 	    }
 	}
+	else
+	{
+	    last_attached_pgrp = pgrp;
+	}
     }
 }