Subshell exiting, suspend problem

Subshell exiting, suspend problem

[Nicolas George]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

There seems to be a problem with zsh process group handling: when zsh is
invoked as an interactive subshell from a curses-based program (by
example with :shell from vim, but the same is true with mutt, less,
flrn...), when zsh exits, the calling process gets suspended for "tty
output" (only if it is under job control, or else it makes a read
error).

The problem arises with zsh 4.1.1, or with a CVS snapshot from half an
hour ago, and also with the Debian-patched 4.0.7.

I have tracked the problem, and it seems that acquire_pgrp() is called
from init_io(), but release_pgrp() is never called. The following patch
fixes the problem:


--- Src/builtin.c	2003-09-26 16:16:45.000000000 +0200
+++ Src/builtin.c.orig	2003-09-26 16:16:09.000000000 +0200
@@ -3977,9 +3977,6 @@
     if (sigtrapped[SIGEXIT])
 	dotrap(SIGEXIT);
     runhookdef(EXITHOOK, NULL);
-    if (opts[MONITOR] && interact && (SHTTY != -1)) {
-	release_pgrp();
-    }
     if (mypid != getpid())
 	_exit(val);
     else


But I am not quite sure if this is exactly the right thing: maybe the
correct condition is to call release_pgrp() if and only if
acquire_pgrp() was called. The only thing I am sure is that something
like that is necessary.

Regards,

-- 
  Nicolas George

[-- Attachment #2: Type: application/pgp-signature, Size: 185 bytes --]

Re: Subshell exiting, suspend problem

[Bart Schaefer]

On Sep 26,  6:52pm, Nicolas George wrote:
} 
} There seems to be a problem with zsh process group handling: when zsh is
} invoked as an interactive subshell from a curses-based program (by
} example with :shell from vim, but the same is true with mutt, less,
} flrn...), when zsh exits, the calling process gets suspended for "tty
} output" (only if it is under job control, or else it makes a read
} error).

I was concerned that this would happen when Philippe Troin's patch from
zsh-workers/18319 was applied, but at the time I couldn't find a concrete
example.  I've been of the opinion that, except e.g. when zsh is started
by (the equivalent of) /bin/login as the first process on a terminal, it
should be up to the process that is starting zsh to decide what pgrp zsh
goes into, and zsh has no business acquiring anything.

The question is whether it's more important to work around buggy "su"
implementations, or to have philosophically correct behavior.
 
} [...] release_pgrp() is never called. The following patch
} fixes the problem:
[...]
} But I am not quite sure if this is exactly the right thing: maybe the
} correct condition is to call release_pgrp() if and only if
} acquire_pgrp() was called.

What effect does release_pgrp() have on any jobs started by zsh that are
still running in the background?  I suspect it would disown them.  This
is why I think zsh should not have called acquire_pgrp() in the first
place -- those background jobs should be in the pgrp of whatever is in
control of the terminal after zsh exits, so that they still receive any
HUP signals etc. when that parent process finally exits.

Re: Subshell exiting, suspend problem

[Philippe Troin]

Nicolas George <nicolas.george@ens.fr> writes:

> There seems to be a problem with zsh process group handling: when zsh is
> invoked as an interactive subshell from a curses-based program (by
> example with :shell from vim, but the same is true with mutt, less,
> flrn...), when zsh exits, the calling process gets suspended for "tty
> output" (only if it is under job control, or else it makes a read
> error).

True indeed.

> The problem arises with zsh 4.1.1, or with a CVS snapshot from half an
> hour ago, and also with the Debian-patched 4.0.7.
> 
> I have tracked the problem, and it seems that acquire_pgrp() is called
> from init_io(), but release_pgrp() is never called. The following patch
> fixes the problem:
> 
> 
> --- Src/builtin.c	2003-09-26 16:16:45.000000000 +0200
> +++ Src/builtin.c.orig	2003-09-26 16:16:09.000000000 +0200
> @@ -3977,9 +3977,6 @@
>      if (sigtrapped[SIGEXIT])
>  	dotrap(SIGEXIT);
>      runhookdef(EXITHOOK, NULL);
> -    if (opts[MONITOR] && interact && (SHTTY != -1)) {
> -	release_pgrp();
> -    }
>      if (mypid != getpid())
>  	_exit(val);
>      else

Your patch seems to be reversed.
 
> But I am not quite sure if this is exactly the right thing: maybe the
> correct condition is to call release_pgrp() if and only if
> acquire_pgrp() was called. The only thing I am sure is that something
> like that is necessary.

No, you're right, release_pgrp() ought to be called upon exit. Good
catch.

I am going to run some extra tests before confirming this patch. I'm
also not sure we need all the extra tests (opts[MONITOR], etc).

Phil.

Re: Subshell exiting, suspend problem

[Philippe Troin]

Bart Schaefer <schaefer@brasslantern.com> writes:

> On Sep 26,  6:52pm, Nicolas George wrote:
> } 
> } There seems to be a problem with zsh process group handling: when zsh is
> } invoked as an interactive subshell from a curses-based program (by
> } example with :shell from vim, but the same is true with mutt, less,
> } flrn...), when zsh exits, the calling process gets suspended for "tty
> } output" (only if it is under job control, or else it makes a read
> } error).
> 
> I was concerned that this would happen when Philippe Troin's patch from
> zsh-workers/18319 was applied, but at the time I couldn't find a concrete
> example.  I've been of the opinion that, except e.g. when zsh is started
> by (the equivalent of) /bin/login as the first process on a terminal, it
> should be up to the process that is starting zsh to decide what pgrp zsh
> goes into, and zsh has no business acquiring anything.
> 
> The question is whether it's more important to work around buggy "su"
> implementations, or to have philosophically correct behavior.

The patch does much more than work around buggy su's :-) It also makes
job control work when zsh is spawned by an non-shell, non-login
program.

> } [...] release_pgrp() is never called. The following patch
> } fixes the problem:
> [...]
> } But I am not quite sure if this is exactly the right thing: maybe the
> } correct condition is to call release_pgrp() if and only if
> } acquire_pgrp() was called.
> 
> What effect does release_pgrp() have on any jobs started by zsh that are
> still running in the background?  I suspect it would disown them.

I don't get it. Release_pgrp() only ought to be called when zsh
terminates one way or the other. The current code calls
(acquire|release)_pgrp():

  - on startup, acquire_pgrp is called.

  - upon exec, release_pgrp is called

  - upon suspend release_pgrp is called, and when zsh is resumed
    acquire_pgrp is called again.

  - when clone is called (mostly irrelevant for the present
    discussion)

I forgot (blame me):

  - call release_pgrp() on exit, hence the bug.

As far as calling release_pgrp() with jobs in the background:

  - when suspend is called from a zsh which has processes in the
    background: for the backgrounded processes, it is strictly
    equivalent to zsh putting another job in the foreground.

  - the only other case where release_pgrp() is called is upon exit
    (at least after this patch has been applied), and the jobs will
    continue, orphaned...

> This is why I think zsh should not have called acquire_pgrp() in the
> first place -- those background jobs should be in the pgrp of
> whatever is in control of the terminal after zsh exits, so that they
> still receive any HUP signals etc. when that parent process finally
> exits.

AFAIR, SIGHUP only gets sent to all members of a session when the
session leader exits. It is not related to process groups. So
acquire/release_pgrp() are irrelevant in this instance since these
background jobs are part of the same session. How is the situation you
describe different from (say) a subshell spawned from zsh directly?

Phil.

Re: Subshell exiting, suspend problem

[Bart Schaefer]

On Sep 26,  8:21pm, Philippe Troin wrote:
}
} AFAIR, SIGHUP only gets sent to all members of a session when the
} session leader exits. It is not related to process groups.

It depends on the antiquity of your operating system, actually.  There
is AFAIK no distinction between sessions and process groups in some
versions of unix (i.e., a session is a process group that has a TTY).
I forget when setsid() et al. were introduced, but it is not universal
to all OSs on which zsh compiles.

} So acquire/release_pgrp() are irrelevant in this instance since these
} background jobs are part of the same session.

That may be true for the HUP on session leader exit, but what about e.g.
TSTP when the user types ^Z, or INT on ^C ?

E.g. if bash starts mutt which starts zsh which starts vi, and then the
user types ^Z, I believe it should be up to mutt to have placed zsh in
its own process group if mutt did not want the TSTP.  In this case I
don't think zsh has any business forcing itself to be the group leader,
even if it is interactive.

However, there are enough programs that don't pay attention to this
detail when spawning a child process that it may be preferable for zsh
to ignore this philosophical nicety, especially when job control is
enabled.

} How is the situation you describe different from (say) a subshell
} spawned from zsh directly?

Depends what you mean by a "subshell" -- I prefer to restrict that term
to the shell syntactic construct of placing a command in parens, not to
explicitly starting a new shell by name from the prompt of another.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: [19140] Subshell exiting, suspend problem

[Danek Duvall]

I've been using this patch for a little while on a Solaris box, and it
solved the problem I had where su would be left in the job table after I
exited it.

However, I'm now unable to suspend vi or ncftp, or, I would guess, any
curses program.  Moreover, if I run a copy of zsh without this patch
from the commandline of one with the patch, when I exit, I get:

    zsh: can't set tty pgrp: I/O error
    <prompt>% 
    zsh: error on TTY read: I/O error

and the "bottom copy" exits, terminating my login session.

So it seems like there are a few wrinkles to work out yet.  :)

Thanks,
Danek

Re: [19140] Subshell exiting, suspend problem

[Philippe Troin]

Danek Duvall <duvall@emufarm.org> writes:

> I've been using this patch for a little while on a Solaris box, and it
> solved the problem I had where su would be left in the job table after I
> exited it.

Can you elaborate on that problem? I use zsh with su on solaris
without any issues.
 
> However, I'm now unable to suspend vi or ncftp, or, I would guess, any
> curses program.

That's strange: this patch only changes zsh's exit behavior.

> Moreover, if I run a copy of zsh without this patch
> from the commandline of one with the patch, when I exit, I get:
> 
>     zsh: can't set tty pgrp: I/O error
>     <prompt>% 
>     zsh: error on TTY read: I/O error
> 
> and the "bottom copy" exits, terminating my login session.
> 
> So it seems like there are a few wrinkles to work out yet.  :)

I'm working on that, or better, trying to find some time to do that.

Phil.

Re: Subshell exiting, suspend problem

[Danek Duvall]

On Fri, Oct 03, 2003 at 03:06:09PM -0700, Philippe Troin wrote:

> Danek Duvall <duvall@emufarm.org> writes:
> 
> > I've been using this patch for a little while on a Solaris box, and
> > it solved the problem I had where su would be left in the job table
> > after I exited it.
> 
> Can you elaborate on that problem? I use zsh with su on solaris
> without any issues.

"su root -c /path/to/zsh" runs the shell.  When I exit, I get

    zsh: suspended (tty output)  su root -c /path/to/zsh

which is confirmed by jobs.  If I foreground the job, it finally exits
properly.  This is if both the user and the root shell are 4.1.1 without
the patch.

If the root shell is patched, then this problem doesn't happen (doesn't
matter what the user shell is).

If the user shell is patched but the root shell is not, then I get the
I/O error I described before.

> > However, I'm now unable to suspend vi or ncftp, or, I would guess,
> > any curses program.
> 
> That's strange: this patch only changes zsh's exit behavior.

Indeed.  I'll recompile both the patched version and the unpatched
version from scratch and see if I can reproduce all this behavior.

Thanks,
Danek

Re: Subshell exiting, suspend problem

[Danek Duvall]

On Fri, Oct 03, 2003 at 03:24:43PM -0700, Danek Duvall wrote:

> On Fri, Oct 03, 2003 at 03:06:09PM -0700, Philippe Troin wrote:
> 
> > Danek Duvall <duvall@emufarm.org> writes:
> >
> > > However, I'm now unable to suspend vi or ncftp, or, I would guess,
> > > any curses program.
> > 
> > That's strange: this patch only changes zsh's exit behavior.
> 
> Indeed.  I'll recompile both the patched version and the unpatched
> version from scratch and see if I can reproduce all this behavior.

Mea culpa.  I'd run configure in the background, and it failed to detect
tcsetpgrp properly.  Rebuilding in the foreground solved the issue.

Thanks,
Danek

Re: Subshell exiting, suspend problem

[Philippe Troin]

Danek Duvall <duvall@emufarm.org> writes:

> On Fri, Oct 03, 2003 at 03:24:43PM -0700, Danek Duvall wrote:
> 
> > On Fri, Oct 03, 2003 at 03:06:09PM -0700, Philippe Troin wrote:
> > 
> > > Danek Duvall <duvall@emufarm.org> writes:
> > >
> > > > However, I'm now unable to suspend vi or ncftp, or, I would guess,
> > > > any curses program.
> > > 
> > > That's strange: this patch only changes zsh's exit behavior.
> > 
> > Indeed.  I'll recompile both the patched version and the unpatched
> > version from scratch and see if I can reproduce all this behavior.
> 
> Mea culpa.  I'd run configure in the background, and it failed to detect
> tcsetpgrp properly.  Rebuilding in the foreground solved the issue.

Glad to hear that :-)

I'll see if I can fix the configure script for that case. At worst, I
can make it fail if ran in the background.

Phil.

Re: Subshell exiting, suspend problem

[Philippe Troin]

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

Philippe Troin <phil@fifi.org> writes:

> Danek Duvall <duvall@emufarm.org> writes:
> 
> > On Fri, Oct 03, 2003 at 03:24:43PM -0700, Danek Duvall wrote:
> > 
> > > On Fri, Oct 03, 2003 at 03:06:09PM -0700, Philippe Troin wrote:
> > > 
> > > > Danek Duvall <duvall@emufarm.org> writes:
> > > >
> > > > > However, I'm now unable to suspend vi or ncftp, or, I would guess,
> > > > > any curses program.
> > > > 
> > > > That's strange: this patch only changes zsh's exit behavior.
> > > 
> > > Indeed.  I'll recompile both the patched version and the unpatched
> > > version from scratch and see if I can reproduce all this behavior.
> > 
> > Mea culpa.  I'd run configure in the background, and it failed to detect
> > tcsetpgrp properly.  Rebuilding in the foreground solved the issue.
> 
> Glad to hear that :-)
> 
> I'll see if I can fix the configure script for that case. At worst, I
> can make it fail if ran in the background.

Danek,

Can you check that you can reproduce the bit about failing to detect
tcsetpgrp properly?

If you can reproduce it, can you then try this patch and see if it
fixes the configure problem? You will need to regenerate the configure
script by running "autoconf" after patching zshconfig.ac.

BTW, I could not reproduce your problem at all. If I run configure in
the background, it just stops during the "checking if tcsetpgrp()
actually works..." test. This patch fixes this part.

Thanks,
Phil.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: zsh-tcsetgrp-bgconfigure.patch --]
[-- Type: text/x-patch, Size: 700 bytes --]

Index: zshconfig.ac
===================================================================
RCS file: /cvsroot/zsh/zsh/zshconfig.ac,v
retrieving revision 1.41
diff -b -u -r1.41 zshconfig.ac
--- zshconfig.ac	15 Sep 2003 09:20:21 -0000	1.41
+++ zshconfig.ac	17 Oct 2003 16:47:40 -0000
@@ -1614,6 +1614,7 @@
 dnl for instance, BeOS R4.51 does not support it yet
 dnl -----------
 if test -t 0 && test $ac_cv_func_tcsetpgrp = yes; then
+    trap "" SIGTTOU
     AC_CACHE_CHECK(if tcsetpgrp() actually works,
     zsh_cv_sys_tcsetpgrp,
     [AC_TRY_RUN([
@@ -1631,6 +1632,7 @@
     if test $zsh_cv_sys_tcsetpgrp = no; then
       AC_DEFINE(BROKEN_TCSETPGRP)
     fi
+    trap - SIGTTOU
 fi
 
 dnl -----------

Re: Subshell exiting, suspend problem

[Danek Duvall]

On Fri, Oct 17, 2003 at 09:54:55AM -0700, Philippe Troin wrote:

> Can you check that you can reproduce the bit about failing to detect
> tcsetpgrp properly?

I can't.  At this point, it's been long enough that I can't remember
what I might have done it to push it one way or the other.

However ... I plucked the test to check if tcsetpgrp() works or not out,
and ran it with stdin redirected from /dev/null, and it promptly fails.
The test is only run, however, if test -t 0 is true.  I must have
somehow hooked stdin up to something that was a terminal, but wasn't the
controlling terminal?

I'm a bit confused, especially since our build script has changed
recently to redirect all stdin from /dev/null, but I'm not using that
version of the build script, etc.  At any rate, I've lost the ability to
reproduce what I did, unless I stumble across the magic incantation
again.

> BTW, I could not reproduce your problem at all. If I run configure in
> the background, it just stops during the "checking if tcsetpgrp()
> actually works..." test. This patch fixes this part.

Yeah, I still see that.

If I have time, and can reproduce the problem, I'll send out another
message. If I don't, I'll just mutter quietly in a dark corner
somewhere.  :)

Thanks,
Danek