help / color / mirror / code / Atom feed
From: Jun T <takimoto-j@kba.biglobe.ne.jp>
To: zsh-workers@zsh.org
Subject: Re: zargs with -P intermittently failing in zsh 5.9 and macOS
Date: Fri, 27 May 2022 14:29:37 +0900	[thread overview]
Message-ID: <8CB92976-5B21-4239-844E-93C88EC734F5@kba.biglobe.ne.jp> (raw)
In-Reply-To: <CAH+w=7YQmKzEXFHwRwLF2Df9E51G-Jmof+NtkK3Vpkv-pkGx8A@mail.gmail.com>

> 2022/05/27 12:39, Bart Schaefer <schaefer@brasslantern.com> 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

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));

  parent reply	other threads:[~2022-05-27  5:30 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-24 16:09 Eric Nielsen
2022-05-24 17:32 ` Bart Schaefer
2022-05-24 20:21   ` Eric Nielsen
2022-05-24 20:35     ` Bart Schaefer
2022-05-26  8:57   ` Jun T
2022-05-26 11:21     ` Jun. T
2022-05-27  3:39     ` Bart Schaefer
2022-05-27  4:18       ` Lawrence Velázquez
2022-05-27  5:29       ` Jun T [this message]
2022-05-27 16:10         ` Bart Schaefer
2022-05-27 17:25           ` Jun. T
2022-05-29  3:30             ` Bart Schaefer
2022-05-29 14:47               ` Jun. T
2022-05-29 20:07                 ` Bart Schaefer
2022-05-30 11:16                   ` Jun T
2022-06-07  6:51                     ` Jun T
2022-06-09  2:56                       ` Bart Schaefer
2022-06-12 16:43                 ` Eric Nielsen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8CB92976-5B21-4239-844E-93C88EC734F5@kba.biglobe.ne.jp \
    --to=takimoto-j@kba.biglobe.ne.jp \
    --cc=zsh-workers@zsh.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).