Re: long-standing tty related issue: wrapped Emacs not suspended

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

On Fri, 21 Sep 2018 17:57:40 +0100
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx> wrote:
> On Thu, 20 Sep 2018 14:30:05 +0200
> Vincent Lefevre <vincent@xxxxxxxxxx> wrote:
> > is suspended as expected. But when Emacs is wrapped in a function,
> > it is not suspended. After "zsh -f":
> > 
> > zira% e() { emacs -nw "$@"; }
> > zira% e &
> > 
> > I cannot quit Emacs or get the zsh prompt. I need to kill the
> > terminal.  
> 
> I think this one *is* the same basic issue that Thilo was talking about.
> As you note, it's not new.  (With the latest code I can only get this
> the way Thilo did, by suspending, putting it into the background, then
> into the foreground, then I can only get things to work again with kill
> -CONT from elsewhere and a bit of massaging to kill of the remaining
> goo.   The code shown above actually works OK for me with a bg and fg.)

This fixes this variant of the bug for me.  However, there is a lot of
confusion over who is seeing what variant, so it probably doesn't fix
everything --- I have mostly modified code associated with bg and fg
plus some quite well-hidden stuff to do with superjobs.  If you don't
know what a superjob is you should seriously consider keeping it that
way.

Although the changes look individually sane, it's perfectly possible
there are undesirable knock-on effects.  I intend to commit this anyway
as otherwise we won't find out.  If we are going to find out, it needs
to be subjected to some interactive job control usage *now*, *NOT* after
the next release, which doesn't work as a procedure.

All three parts were needed:

- Send SIGCONT when attaching a subjob to the TTY.  This is because
its stoppedness is invisible to the user, who is (unknowlingly) only
directly manipulating the superjob, so the shell has to give a helping
hand.  Comment added.

- Wait for subjob of superjob if waiting for the superjob, else the "fg"
makes the top-level shell continue immediately, so both jobs are running
at once.  (The wait for the superjob returns when that exits, even
though the only process, the forked shell, is stopped at this point.
It's not clear to me this is correct --- see note on makerunning()
below.  I think with this behaviour waiting for the subjob may be
superfluous, but I think it's correct anyway as the whole lot is
considered to be in the forground.)

- Mark a job as no longer STAT_STOPPED if we send it SIGCONT --- do this
generically from within the killjb() where it's already specialised for
SIGCONT and also in the non-superjob branch, hence removing a couple of
cases that did this outside klllb().  Why in goodness name did we not do
this already?

Note I'm not 100% convinced that makerunning() is consistent with the
way killjb() doesn't SIGCONT the forked subshell --- so the superjob
isn't actually fully running and I'm not sure how it should be flagged.
I'm assuming that any change to this needs to go in makerunning(), which
has been made aware of superjobs, so I've used that consistently to
update the status regardless of this query.

pws

diff --git a/Src/jobs.c b/Src/jobs.c
index 2d58319a8..c399f1d3e 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1569,10 +1569,8 @@ zwaitjob(int job, int wait_cmd)
 
            errflag = 0; */
 
-	    if (subsh) {
+	    if (subsh)
 		killjb(jn, SIGCONT);
-		jn->stat &= ~STAT_STOPPED;
-	    }
 	    if (jn->stat & STAT_SUPERJOB)
 		if (handle_sub(jn - jobtab, 1))
 		    break;
@@ -1590,6 +1588,17 @@ zwaitjob(int job, int wait_cmd)
     return 0;
 }
 
+static void waitonejob(Job jn)
+{
+    if (jn->procs || jn->auxprocs)
+	zwaitjob(jn - jobtab, 0);
+    else {
+	deletejob(jn, 0);
+	pipestats[0] = lastval;
+	numpipestats = 1;
+    }
+}
+
 /* wait for running job to finish */
 
 /**/
@@ -1599,13 +1608,10 @@ waitjobs(void)
     Job jn = jobtab + thisjob;
     DPUTS(thisjob == -1, "No valid job in waitjobs.");
 
-    if (jn->procs || jn->auxprocs)
-	zwaitjob(thisjob, 0);
-    else {
-	deletejob(jn, 0);
-	pipestats[0] = lastval;
-	numpipestats = 1;
-    }
+    waitonejob(jn);
+    if (jn->stat & STAT_SUPERJOB)
+	waitonejob(jobtab + jn->other);
+	
     thisjob = -1;
 }
 
@@ -2294,11 +2300,8 @@ bin_fg(char *name, char **argv, Options ops, int func)
 	    Process p;
 
 	    if (findproc(pid, &j, &p, 0)) {
-		if (j->stat & STAT_STOPPED) {
+		if (j->stat & STAT_STOPPED)
 		    retval = (killjb(j, SIGCONT) != 0);
-		    if (retval == 0)
-			makerunning(j);
-		}
 		if (retval == 0) {
 		    /*
 		     * returns 0 for normal exit, else signal+128
@@ -2404,9 +2407,17 @@ bin_fg(char *name, char **argv, Options ops, int func)
 			((!jobtab[job].procs->next ||
 			  (jobtab[job].stat & STAT_SUBLEADER) ||
 			  killpg(jobtab[job].gleader, 0) == -1)) &&
-			jobtab[jobtab[job].other].gleader)
+			jobtab[jobtab[job].other].gleader) {
 			attachtty(jobtab[jobtab[job].other].gleader);
-		    else
+			/*
+			 * In case stopped by putting in background.
+			 * Usually that's visible to the user, who
+			 * can restart, but with a superjob this is done
+			 * behind the scenes, so do it here.  Should
+			 * be harmless if not needed.
+			 */
+			stopped = 1;
+		    } else
 			attachtty(jobtab[job].gleader);
 		}
 	    }
diff --git a/Src/signals.c b/Src/signals.c
index 26d88abc2..f294049c2 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -782,7 +782,20 @@ killjb(Job jn, int sig)
 		    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
 			err = -1;
 
-                return err;
+		/*
+		 * The following marks both the superjob and subjob
+		 * as running, as done elsewhere.
+		 *
+		 * It's not entirely clear to me what the right way
+		 * to flag the status of a still-pausd final process,
+		 * as handled above, but we should be cnsistent about
+		 * this inside makerunning() rather than doing anything
+		 * special here.
+		 */
+		if (err != -1)
+		    makerunning(jn);
+
+		return err;
             }
             if (killpg(jobtab[jn->other].gleader, sig) == -1 && errno != ESRCH)
 		err = -1;
@@ -792,8 +805,11 @@ killjb(Job jn, int sig)
 
 	    return err;
         }
-        else
-	    return killpg(jn->gleader, sig);
+        else {
+	    err = killpg(jn->gleader, sig);
+	    if (sig == SIGCONT && err != -1)
+		makerunning(jn);
+	}
     }
     for (pn = jn->procs; pn; pn = pn->next) {
 	/*