Re: [PATCH] Do not send duplicate signals when MONITOR is set

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

On Mon, 2021-06-07 at 11:45 -0700, Bart Schaefer wrote:
> On Mon, Jun 7, 2021 at 10:28 AM Erik Paulson <epaulson10@xxxxxxxxx> wrote:
> > 
> > I run emacs as a daemon and use the emacsclient program to connect to
> > it. I noticed that when I suspended the emacsclient program and
> > resumed it in zsh, the program would sporadically crash. After digging
> > into the code, I realized that emacsclient was receiving two SIGCONTs,
> > which caused it to send a malformed command to the daemon.
> > 
> > I found that this return used to be present, but was removed in
> > https://www.zsh.org/mla/workers/2018/msg01338.html while addressing
> > another emacs issue.
> 
> I don't think it was removed ... similar code was added in two
> separate places, but the "return" was only added in one of those.
> 
> Your patch adds that return in the second case.
> 
> The difference is that in the first case, the SIGCONT is received by a
> job that is marked STAT_SUPERJOB and in the second case it's received
> by a different job.
> 
> I believe this means that in the former case the superjob is in the
> foreground and in the second case, it isn't -- rather one of its
> subjobs is.  In the first instance zsh sends the signal to all the
> subjobs and then to the process group.  In the second case it sends
> the signal to the process group first and then falls into the loop
> sending the signal to any subjobs that still appear to be stopped.
> 
> In any case I think a potential problem with placing an unconditional
> "return" where your patch does, is that signals other than SIGCONT
> probably still need to be delivered to the subjobs.  PWS, any input
> here?

Hmm, it's not clear to me in what cases you'd need to deliver both
killpg() to the group leader and then kill to the processes.  You might
hope if the shell was doing the right thing the first would be enough.
But there might be special cases.

I would hazard that as SIGCONT is probably the most difficult case ---
the only one where you specifically want the process to be running
afterwards --- if this patch improves things there, it's prohably not
doing a lot of harm in most cases.

It's not impossible there are some oddities where process groups aren't
set up quite how you want that this might affect, but I doubt we're
going to spot them just by staring.

pws