Re: zargs with -P intermittently failing in zsh 5.9 and macOS

[Jun T <takimoto-j@xxxxxxxxxxxxxxxxx>]

> 2022/05/27 12:39, Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> 
> I don't suppose you've found any indication of what 19 actually means, there.

I think I've found it now; 19 = SIGCONT.

If in subshell, zwaitjob(job) calls killjb(jn, SIGONT) (jobs.c:1657).

When wait_for_processes() is called (in signal handler?), wait3(&state)
_sometimes_ returns with WIFCONTINUED(status) set to true.
Then wait_for_processes() calls addbgstatus(pid, val) with
val=WEXITSTATUS(status)=19.

Later the process exits normally, and addbgstatus(pid, 0) is called.
This means bgstatus_list has two entries for the pid.

When 'wait pid' calls getbgstatus(pid), it finds the entry with status=19
and returns it.

A simple fix would be, at line 584 of signals.c,
(A) call addbgstatus() only if WIFCONTINUED() is not true.

But, what should we do if WIFSTOPPED() is true?

The comment before addbgstatus() (jobs.c, line 2211) says:
   Record the status of a background process that exited so we
   can execute the builtin wait for it.

So I guess we need to call addbgstatus() only if the process has actually
exited (or killed). If this is the case, better solution would be
(B) call addbgstatus() only if WIFEXITED() or WIFSIGNALED() is true.
This it the patch below.

Or, if we want to call addbgstatus() unconditionally,
(C) modify addbgstatus() so that if there is already an entry for
the pid then update it instead of adding a new entry.

I feel (B) is the best way, but not sure. Any idea?


>>    #sleep 1    # works OK if 'sleep 1' is added here
>>    wait        # works OK if this line is commented out
> 
> Hm.  If that wait is removed, the subshell is probably not necessary.
> It's there because of a lingering concern that if we didn't first wait
> for all jobs to finish before starting the individual waits, we might
> get race conditions. It seems like perhaps the opposite is actually
> the case.

I think 'wait pid' for all the pids in $_zajobs would be enough, but
not 100 % sure. zargs need not be modified if the addbgstatus-problem
is fixed (in some way), but if the wait/subshell is not necessary then
it is better removed.

It can happen that a user starts (in a script, probably) many background
jobs and want to wait for only part of the jobs. If waiting for each
job does not work, the user may call it a bug.
# of cause if it is hard to fix, then it would be better to include a
# workaround in zargs.


diff --git a/Src/signals.c b/Src/signals.c
index 5c787e2a8..8096993cd 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -575,14 +575,11 @@ wait_for_processes(void)
 	 * and is not equal to the current foreground job.
 	 */
 	if (jn && !(jn->stat & (STAT_CURSH|STAT_BUILTIN)) &&
-	    jn - jobtab != thisjob) {
-	    int val = (WIFSIGNALED(status) ?
-		   0200 | WTERMSIG(status) :
-		   (WIFSTOPPED(status) ?
-		    0200 | WEXITSTATUS(status) :
-		    WEXITSTATUS(status)));
-	    addbgstatus(pid, val);
-	}
+	    jn - jobtab != thisjob &&
+	    /* record the status only if the process has exited */
+	    (WIFEXITED(status) || WIFSIGNALED(status)))
+	    addbgstatus(pid, WIFSIGNALED(status) ? 0200 | WTERMSIG(status)
+						: WEXITSTATUS(status));
 
 	unqueue_signals();
     }