5.0.8 regression when waiting for suspended jobs

5.0.8 regression when waiting for suspended jobs

[Christian Neukirchen]

Waiting for a job suspened with tty input results in an infinite loop
of messages in 5.0.8, but just one in 5.0.7.

zsh 5.0.7 (x86_64-unknown-linux-gnu)
zsh-5.0.7-0-g208bded
% cat & wait $!
[1] 17845
[1]  + suspended (tty input)  cat

zsh 5.0.8 (x86_64-unknown-linux-gnu)
zsh-5.0.8-0-gf0068ed
% cat & wait $!
[1] 6783
[1]  + suspended (tty input)  cat
[1]  + suspended (tty input)  cat
[1]  + suspended (tty input)  cat
[1]  + suspended (tty input)  cat
...

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

On Jul 30,  4:32pm, Christian Neukirchen wrote:
}
} Waiting for a job suspened with tty input results in an infinite loop
} of messages in 5.0.8, but just one in 5.0.7.

Hm.  Both are busy-waiting, and I don't see the reason why 5.0.7 is NOT
printing the job status every time.

Also if you wait by job number ("wait %1") instead, "wait" returns right
away rather than blocking on the stopped job (in both .7 and .8).

Re: 5.0.8 regression when waiting for suspended jobs

[Christian Neukirchen]

Bart Schaefer <schaefer@brasslantern.com> writes:

> On Jul 30,  4:32pm, Christian Neukirchen wrote:
> }
> } Waiting for a job suspened with tty input results in an infinite loop
> } of messages in 5.0.8, but just one in 5.0.7.
>
> Hm.  Both are busy-waiting, and I don't see the reason why 5.0.7 is NOT
> printing the job status every time.

Note that I pass the PID, just "wait" stops too.
My 4.3.17 also stops.

> Also if you wait by job number ("wait %1") instead, "wait" returns right
> away rather than blocking on the stopped job (in both .7 and .8).

Can confirm.  Why would it be different when giving the PID...

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

On Jul 31, 10:30am, Christian Neukirchen wrote:
}
} > Also if you wait by job number ("wait %1") instead, "wait" returns right
} > away rather than blocking on the stopped job (in both .7 and .8).
} 
} Can confirm.  Why would it be different when giving the PID...

Comparison:

zsh-5.0.7
 - "wait $!" blocks (looping on repeated wait3() nonzero)
 - "wait %1" returns immediately
 - "wait" returns immediately

zsh-5.0.8
 - "wait $!" loops but also printing status every time
 - "wait %1" returns immediately
 - "wait" returns immediately

bash-4.2.25
 - "wait $!" returns immediately
 - "wait %1" returns immediately
 - "wait" returns immediately

ksh93 (2011)
 - "wait $!" blocks
 - "wait %1" blocks
 - "wait" blocks

There's an additional interesting thing about ksh.  If you interrupt
either of "wait $!" or "wait %1", ksh sends the signal along to the
job that was waited for.  Neither bash nor zsh do so (bash can't as
wait has already returned).

Re: 5.0.8 regression when waiting for suspended jobs

[Christian Neukirchen]

Bart Schaefer <schaefer@brasslantern.com> writes:

> On Jul 31, 10:30am, Christian Neukirchen wrote:
> }
> } > Also if you wait by job number ("wait %1") instead, "wait" returns right
> } > away rather than blocking on the stopped job (in both .7 and .8).
> } 
> } Can confirm.  Why would it be different when giving the PID...
>
> Comparison:
>
> zsh-5.0.7
>  - "wait $!" blocks (looping on repeated wait3() nonzero)
>  - "wait %1" returns immediately
>  - "wait" returns immediately
>
> zsh-5.0.8
>  - "wait $!" loops but also printing status every time
>  - "wait %1" returns immediately
>  - "wait" returns immediately

TBH I don't know the best behavior here, but it's not endlessly
looping spewing messages.  :)

> bash-4.2.25
>  - "wait $!" returns immediately
>  - "wait %1" returns immediately
>  - "wait" returns immediately
>
> ksh93 (2011)
>  - "wait $!" blocks
>  - "wait %1" blocks
>  - "wait" blocks

I found it surprising that pid and jobid behave differently, but if
that's for historical reasons, I'm fine with that.

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

On Jul 31,  7:28pm, Christian Neukirchen wrote:
}
} TBH I don't know the best behavior here, but it's not endlessly
} looping spewing messages.  :)

Yes, obviously.  But while we're fixing it we should at least think
about whether to fix any other discrepancies.

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

On Jul 31,  8:56am, Bart Schaefer wrote:
}
} zsh-5.0.7
}  - "wait $!" blocks (looping on repeated wait3() nonzero)
}  - "wait %1" returns immediately
}  - "wait" returns immediately
} 
} zsh-5.0.8
}  - "wait $!" loops but also printing status every time
}  - "wait %1" returns immediately
}  - "wait" returns immediately

I still only suspect what changed to make 5.0.8 different from 5.0.7 in
this regard, but here's what's going on:

- "wait $!" -

bin_fg() calls waitforpid() which discovers the job is stopped and goes
into a loop calling kill(pid, SIGCONT) to try to get the job to run
again.  In the 5.0.8 case, each time this happens the job briefly wakes
up, gets stopped with SIGTTIN, thus causes another SIGCHLD to go to the
parent zsh, which then prints the "suspended" message and loops right
back to kill(pid, SIGCONT) again.

All of this is exactly the same as in 5.0.7 except that because of the
SIGCONT change in workers/35032 we notice the stopped -> continued ->
stopped again status change and therefore print the new status even
though it's actually the same as the last time we printed the status,
because we skipped printing the "continued" status.  Or so I surmise.

- wait %1" -

bin_fg() calls zwaitjob() which does NOT do kill(pid, SIGCONT) instead
simply blocking forever waiting for a SIGCHLD that will never arrive.

If a signal *is* received and the waiting shell is a subshell, *then*
the awaited job is SIGCONT'd, but I don't recall why and it doesn't
matter for this bug anyway.

This does however raise the question of why zwaitjob() is not calling
waitforpid().  If it did so, we'd have the ksh behavior for all three
cases of "wait", and we could even add the bit where interrupting the
wait sends the signal through to the waited-for job.

- "wait" -

bin_fg() goes into a loop calling zwaitjob() on every entry in the job
table; i.e., identical to "wait %1" repeated for every job number.

======

So what do we do about this?  Skip the SIGCONT in waitforpid()?  Only
try SIGCONT once in waitforpid() rather than every time around the
loop?  Some other thing involving the WIFCONTINUED() test?  Assuming
we work that out, should zwaitjob() be changed to use waitforpid(), or
do we think someone is relying on the bash-like immediate return?

Re: 5.0.8 regression when waiting for suspended jobs

[Peter Stephenson]

On Tue, 11 Aug 2015 16:56:55 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jul 31,  8:56am, Bart Schaefer wrote:
> I still only suspect what changed to make 5.0.8 different from 5.0.7 in
> this regard, but here's what's going on:

> - "wait $!" -
> } zsh-5.0.7
> }  - "wait $!" blocks (looping on repeated wait3() nonzero)
> } zsh-5.0.8
> }  - "wait $!" loops but also printing status every time
> 
> bin_fg() calls waitforpid() which discovers the job is stopped and goes
> into a loop calling kill(pid, SIGCONT) to try to get the job to run
> again.  In the 5.0.8 case, each time this happens the job briefly wakes
> up, gets stopped with SIGTTIN, thus causes another SIGCHLD to go to the
> parent zsh, which then prints the "suspended" message and loops right
> back to kill(pid, SIGCONT) again.
> 
> All of this is exactly the same as in 5.0.7 except that because of the
> SIGCONT change in workers/35032 we notice the stopped -> continued ->
> stopped again status change and therefore print the new status even
> though it's actually the same as the last time we printed the status,
> because we skipped printing the "continued" status.  Or so I surmise.

So you might have thought the right thing to do was note it had been
stopped immediately, possibly warn the user, and not try to continue it
again without further user action?  Is that easy?  Can we pin down
"immediately" well enough?  Clearly there's a race in the real world
where the programme could get SIGTTIN at any time, but in the general
case (i.e. where a background process got SIGTTIN when the foreground
was doing something irrelevant) you clearly *don't* want it to continue
every time.

In that case the difference between 5.0.7 and 5.0.8 becomes
basically moot (it's different but in a sane fashion).

Do we even understand what the loop with SIGCONT is doing for us?  Under
what circumstances would this help?  Some (other sort of) race where
something else (what?  Not zsh and not the process that's suspended)
takes a while to get going, so the SIGCONT only succeeds after a few
attempts?

> - wait %1" -
> 
> bin_fg() calls zwaitjob() which does NOT do kill(pid, SIGCONT) instead
> simply blocking forever waiting for a SIGCHLD that will never arrive.

Hmm... I can't think of a good reason from the user point of view why
this should behave differently.  It just seems confusing.  It's
certainly not documented as a zsh feature, is it?

> - "wait" -
> 
> bin_fg() goes into a loop calling zwaitjob() on every entry in the job
> table; i.e., identical to "wait %1" repeated for every job number.

In which case I think the same reaction arises.

pws

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

Preface:  I think I've figured out why zwaitjob() does not use the same
logic as waitforpid(); zwaitjob() may be waiting for an entire pipeline,
needing to record status of multiple actual processes which may exit in
any order, and only finish when all the processes are complete.

On Aug 12, 10:43am, Peter Stephenson wrote:
} Subject: Re: 5.0.8 regression when waiting for suspended jobs
}
} On Tue, 11 Aug 2015 16:56:55 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > } zsh-5.0.7
} > }  - "wait $!" blocks (looping on repeated wait3() nonzero)
} > } zsh-5.0.8
} > }  - "wait $!" loops but also printing status every time
} > 
} > bin_fg() calls waitforpid() which discovers the job is stopped and goes
} > into a loop calling kill(pid, SIGCONT) to try to get the job to run
} > again.
} > 
} > All of this is exactly the same as in 5.0.7 except that because of the
} > SIGCONT change in workers/35032 we notice the stopped -> continued ->
} > stopped again status change and therefore print the new status
} 
} So you might have thought the right thing to do was note it had been
} stopped immediately, possibly warn the user, and not try to continue it
} again without further user action?  Is that easy?

No, not really.  I suppose we could do something baroque like examine
the rusage cputime but otherwise the CHLD could be arriving at any point.

We could special-case the SIGTT* signals, we obviously know (from the
status that's printed) which signal stopped the job.

} Clearly there's a race in the real world
} where the programme could get SIGTTIN at any time, but in the general
} case (i.e. where a background process got SIGTTIN when the foreground
} was doing something irrelevant) you clearly *don't* want it to continue
} every time.

This only happens for the "wait" command, not for handling the signal
while something else is in the foreground.  There might be some weird
edge case where you could cause it to happen with command substitution
(the only other place waitforpid() is used) but I can't come up with it.

} Do we even understand what the loop with SIGCONT is doing for us?  Under
} what circumstances would this help?

It would seem that it's trying very hard not to have "wait" either fail
immediately (bash behavior) or block forever (ksh behavior).  Doing the
ksh thing makes a bit of sense when "wait" will propagate the signals
(so interrupting wait also interrupts the stopped job).

} Some (other sort of) race where something else (what? Not zsh and
} not the process that's suspended) takes a while to get going, so the
} SIGCONT only succeeds after a few attempts?

Reasoning lost to history, I fear (predates source code control).

} > - wait %1" -
} > 
} > bin_fg() calls zwaitjob() which does NOT do kill(pid, SIGCONT) instead
} > simply blocking forever waiting for a SIGCHLD that will never arrive.

I actually got this one wrong -- yes, zwaitjob() would block forever if
it reached that signal_suspend() call, but in fact it won't even try if
the job status is STAT_STOPPED.  It just silently returns.

-- 
Barton E. Schaefer

Re: 5.0.8 regression when waiting for suspended jobs

[Peter Stephenson]

On Wed, 12 Aug 2015 07:58:58 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> We could special-case the SIGTT* signals, we obviously know (from the
> status that's printed) which signal stopped the job.

That kind of makes sense, since very few if any(?) other signals have
this retry-and-fail-again-immediately behaviour.  Many other IO-related
signals are permanent failures, anyway.

pws

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

On Aug 12,  5:09pm, Peter Stephenson wrote:
} Subject: Re: 5.0.8 regression when waiting for suspended jobs
}
} On Wed, 12 Aug 2015 07:58:58 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > We could special-case the SIGTT* signals, we obviously know (from the
} > status that's printed) which signal stopped the job.
} 
} That kind of makes sense, since very few if any(?) other signals have
} this retry-and-fail-again-immediately behaviour.  Many other IO-related
} signals are permanent failures, anyway.

This is a bit ugly, suggestions welcome.  In the "is a child but has no
job table entry" case, this won't SIGCONT the child where previous it
would have, so there's that.

diff --git a/Src/jobs.c b/Src/jobs.c
index ed647b8..ef2bbf0 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1379,16 +1379,26 @@ int
 waitforpid(pid_t pid, int wait_cmd)
 {
     int first = 1, q = queue_signal_level();
+    Process pn;
 
     /* child_block() around this loop in case #ifndef WNOHANG */
     dont_queue_signals();
     child_block();		/* unblocked in signal_suspend() */
     queue_traps(wait_cmd);
     while (!errflag && (kill(pid, 0) >= 0 || errno != ESRCH)) {
-	if (first)
-	    first = 0;
-	else
-	    kill(pid, SIGCONT);
+	if (first) {
+	    Job jn;
+	    /* We may be waiting for a pid that is a child
+	     * but does not have a job table entry? */
+	    first = !findproc(pid, &jn, &pn, 0);
+	}
+	else if (!WIFSTOPPED(pn->status) ||
+		 (WSTOPSIG(pn->status) != SIGTTIN &&
+		  WSTOPSIG(pn->status) != SIGTTOU))
+	    kill(pid, SIGCONT);		/* Harmless if not stopped */
+	/*
+	  else break;  This is bash behavior, just give up when stopped
+	*/
 
 	last_signal = -1;
 	signal_suspend(SIGCHLD, wait_cmd);

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

Long ramble with a question at the end.

On Aug 12, 10:31am, Bart Schaefer wrote:
}
} This is a bit ugly, suggestions welcome.  In the "is a child but has no
} job table entry" case, this won't SIGCONT the child where previous it
} would have, so there's that.

I've been trying to clean this up and have some ideas, but it occurs to
me that kill(pid, SIGCONT) might not be harmless if the process belongs
to the same user but is not a child of the current shell, or even if it
is a child of the current shell but for some reason has trapped CONT.

Practically speaking "wait" never gets as far as calling waitforpid()
on a process that is not a child, so we shouldn't need to worry about
that except in the abstract (e.g., some future programmer decides to
call waitforpid() in some other context).

However waitforpid() is used by getoutput() and getoutputfile() which
do not create job table entries, so we DO need to deal with that, and
if one of those is stopped it seems wise to continue it.  Problem is
we can't tell whether one of those is stopped or not, so it's a toss-
up whether to send the SIGCONT.

Doing so only on the second+ time around the loop (the old behavior)
seems to be making the assumption that we got SIGCHLD as a result of
the job being stopped.  If we got SIGCHLD because the job actually
exited, we'd have failed the kill(pid, 0) on the next attempt and
left the loop.  However, if the job is already stopped before we go
into waitpid(), that SIGCHLD has likely already been processed, so
we'll block in signal_suspend() without sending SIGCONT.

[Another bash aside --

"wait $pid" for a stopped job returns immediately with no message,
but:

$ wait %1
bash: warning: wait_for_job: job 1 is stopped

This warning doesn't appear if the wait is already running at the
time the child is stopped; in that case you get "[1]+  Stopped ..."
as if the job had been running in the foreground.]

Now consider this pathological case:

% ( while : ; do kill -STOP $sysparams[pid]; done ) &

Even with the SIGTT* test in place, waiting for that is an infinite
loop if SIGCONT is being sent.

Finally, and here's the kicker, bin_fg() ALREADY attempts to SIGCONT
the job before calling waitforpid().


Thus I think we have two choices:

(1) Go for a minimal change of sending the SIGCONT only when !wait_cmd,
which ends up with us blocking forever as with 5.0.7 and before (but
blocked on signal_suspend() rather than busy-waiting, which is good).

(2) Actually determine whether the job is WIFSTOPPED() and break the
loop; the job status is printed when returning to the prompt.

Patch here for the minimal change.  Man that's a lot of analysis to
come up with a two-word edit.


diff --git a/Src/jobs.c b/Src/jobs.c
index ed647b8..b47ba8c 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1384,10 +1384,17 @@ waitforpid(pid_t pid, int wait_cmd)
     dont_queue_signals();
     child_block();		/* unblocked in signal_suspend() */
     queue_traps(wait_cmd);
+
+    /* This function should never be called with a pid that is not a
+     * child of the current shell.  Consequently, if kill(0, pid)
+     * fails here with ESRCH, the child has already been reaped.  In
+     * the loop body, we expect this to happen in signal_suspend()
+     * via zhandler(), after which this test terminates the loop.
+     */
     while (!errflag && (kill(pid, 0) >= 0 || errno != ESRCH)) {
 	if (first)
 	    first = 0;
-	else
+	else if (!wait_cmd)
 	    kill(pid, SIGCONT);
 
 	last_signal = -1;

Re: 5.0.8 regression when waiting for suspended jobs

[Peter Stephenson]

On Fri, 14 Aug 2015 18:56:31 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> (1) Go for a minimal change of sending the SIGCONT only when !wait_cmd,
> which ends up with us blocking forever as with 5.0.7 and before (but
> blocked on signal_suspend() rather than busy-waiting, which is good).

I think I could live with this.

It's getting to the point where it doesn't look all that helpful to
agonise much further unless users have postgraduate degrees in job
control software.

pws

Re: 5.0.8 regression when waiting for suspended jobs

[Bart Schaefer]

On Aug 16,  6:37pm, Peter Stephenson wrote:
} Subject: Re: 5.0.8 regression when waiting for suspended jobs
}
} On Fri, 14 Aug 2015 18:56:31 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > (1) Go for a minimal change of sending the SIGCONT only when !wait_cmd,
} 
} I think I could live with this.

That's good, because I already pushed it!

-- 
Barton E. Schaefer