* Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze
@ 2013-12-20 19:24 Axel Beckert
2013-12-20 20:27 ` Bart Schaefer
0 siblings, 1 reply; 17+ messages in thread
From: Axel Beckert @ 2013-12-20 19:24 UTC (permalink / raw)
To: zsh-workers
Hi,
this has been reported[1] against zsh 5.0.3 in Debian today. Looks like
some regression in 5.0.3 on a first glance. Maybe related to the
test-suite freezes which have been reported on a few architectures.
[1] http://bugs.debian.org/732726
Since 5.0.4 seems close, I thought I'd better forward it soon despite
I haven't looked at it closer yet.
----- Forwarded message from Vincent Lefevre <vincent@vinc17.net> -----
Date: Fri, 20 Dec 2013 19:14:36 +0100
From: Vincent Lefevre <vincent@vinc17.net>
Subject: [Pkg-zsh-devel] Bug#732726: zsh function freeze
Reply-To: Vincent Lefevre <vincent@vinc17.net>, 732726@bugs.debian.org
Package: zsh
Version: 5.0.3-1
Severity: important
This is an important regression (no problems after downgrading to
5.0.2-6), always reproducible.
With a private SVN repository, "svncdiff -c65935 ~/wd | head" freezes, where
svncdiff is the following zsh function (I have "autoload -U svncdiff"):
------------------------------------------------------------------------
#!/usr/bin/env zsh
# Wrapper to "svn diff", written by Vincent Lefevre <vincent@vinc17.org>
# Needs my tdiff utility to process the diff output; otherwise you need
# to remove the "| tdiff" at the end.
# Example of svncdiff usage:
# svncdiff -5 -x -p file
# for 5 lines of unified context and function information.
emulate -LR zsh
local -a args xopt
setopt EXTENDED_GLOB
while [[ $# -ge 1 ]]
do
if [[ "x$1" == x-[0-9]# ]] then
args=($args --diff-cmd diff)
xopt=($xopt -U${1[2,-1]})
elif [[ $# -ge 2 && "x$1" == x-x ]] then
shift
xopt=($xopt $1)
else
args=($args $1)
fi
shift
done
[[ $#xopt -ge 1 ]] && args=(-x "$xopt" $args)
svnwrapper diff "$args[@]" | tdiff
# $Id: svncdiff 38442 2010-08-05 11:41:16Z vinc17/ypig $
------------------------------------------------------------------------
The dependencies can be found on <https://www.vinc17.net/unix/>.
When svncdiff is called as a script, this is no such problem.
With my tps utility, I can observe:
10987 sshd: vlefevre@pts/2
└─> 10988 -zsh
└─> 11262 -zsh
├─> 11264 zsh -f -- /home/vlefevre/bin/svnwrapper diff -c65935 /home/vlefevre/wd
│ └─> 11269 svn diff -c65935 /home/vlefevre/wd
│ ├─> 11273 zsh /home/vlefevre/scripts/ssh mysvn svnserve -t
│ │ ├─> 11295 cat
│ │ └─> 11298 ssh -F /home/vlefevre/.ssh/config -C mysvn svnserve -t
│ └─> 11299 zsh /home/vlefevre/scripts/ssh mysvn svnserve -t
│ ├─> 11317 cat
│ └─> 11320 ssh -F /home/vlefevre/.ssh/config -C mysvn svnserve -t
└─> 11265 perl /home/vlefevre/bin/tdiff
I think that this occurs on big changesets.
$ svncdiff -c65935 ~/wd | wc
2166 14746 212680
I'll try to investigate, but any idea about which zsh change could
trigger the problem?
-- System Information:
Debian Release: jessie/sid
APT prefers unstable
APT policy: (500, 'unstable'), (500, 'testing'), (500, 'stable'), (1, 'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386
Kernel: Linux 3.11-2-amd64 (SMP w/8 CPU cores)
Locale: LANG=POSIX, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Versions of packages zsh depends on:
ii libc6 2.17-97
ii libcap2 1:2.22-1.2
ii libtinfo5 5.9+20130608-1
ii zsh-common 5.0.3-1
Versions of packages zsh recommends:
ii libncursesw5 5.9+20130608-1
ii libpcre3 1:8.31-2
Versions of packages zsh suggests:
ii zsh-doc 5.0.3-1
-- no debconf information
----- End forwarded message -----
----- Forwarded message from Vincent Lefevre <vincent@vinc17.net> -----
Date: Fri, 20 Dec 2013 19:19:18 +0100
From: Vincent Lefevre <vincent@vinc17.net>
To: 732726@bugs.debian.org
Subject: [Pkg-zsh-devel] Bug#732726: zsh function freeze
Reply-To: Vincent Lefevre <vincent@vinc17.net>, 732726@bugs.debian.org
On 2013-12-20 19:14:36 +0100, Vincent Lefevre wrote:
> I think that this occurs on big changesets.
Indeed I can reproduce it with:
svncdiff -c 8540 svn://scm.gforge.inria.fr/svn/mpfr/trunk | head
----- End forwarded message -----
Kind regards, Axel
--
/~\ Plain Text Ribbon Campaign | Axel Beckert
\ / Say No to HTML in E-Mail and News | abe@deuxchevaux.org (Mail)
X See http://www.asciiribbon.org/ | abe@noone.org (Mail+Jabber)
/ \ I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-20 19:24 Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze Axel Beckert @ 2013-12-20 20:27 ` Bart Schaefer 2013-12-20 20:49 ` Axel Beckert 2013-12-20 23:51 ` Vincent Lefevre 0 siblings, 2 replies; 17+ messages in thread From: Bart Schaefer @ 2013-12-20 20:27 UTC (permalink / raw) To: Axel Beckert, zsh-workers On Dec 20, 8:24pm, Axel Beckert wrote: } } Since 5.0.4 seems close, I thought I'd better forward it soon despite } I haven't looked at it closer yet. You're just a tiny bit too late -- PWS announced zsh-5.0.4 about 25 minutes before your email was sent. :-( } this has been reported[1] against zsh 5.0.3 in Debian today. Looks like } some regression in 5.0.3 on a first glance. Maybe related to the } test-suite freezes which have been reported on a few architectures. I think that's unlikely unless svnwrapper is using zpty. I'm not a user of SVN at this point so I don't have a reference. Best at this point to see if you can still reproduce it with the current zsh source from git. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-20 20:27 ` Bart Schaefer @ 2013-12-20 20:49 ` Axel Beckert 2013-12-20 23:51 ` Vincent Lefevre 1 sibling, 0 replies; 17+ messages in thread From: Axel Beckert @ 2013-12-20 20:49 UTC (permalink / raw) To: zsh-workers Hi, On Fri, Dec 20, 2013 at 12:27:00PM -0800, Bart Schaefer wrote: > On Dec 20, 8:24pm, Axel Beckert wrote: > } Since 5.0.4 seems close, I thought I'd better forward it soon despite > } I haven't looked at it closer yet. > > You're just a tiny bit too late -- PWS announced zsh-5.0.4 about 25 > minutes before your email was sent. :-( Hrm, I just saw the git tags so far, but neither me nor Frank Terbeck have received such an announcement via e-mail yet. Well, maybe it's hanging in Greylisting or such. Thanks for the information anyway. :-) Kind regards, Axel -- /~\ Plain Text Ribbon Campaign | Axel Beckert \ / Say No to HTML in E-Mail and News | abe@deuxchevaux.org (Mail) X See http://www.asciiribbon.org/ | abe@noone.org (Mail+Jabber) / \ I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-20 20:27 ` Bart Schaefer 2013-12-20 20:49 ` Axel Beckert @ 2013-12-20 23:51 ` Vincent Lefevre 2013-12-20 23:59 ` Vincent Lefevre 1 sibling, 1 reply; 17+ messages in thread From: Vincent Lefevre @ 2013-12-20 23:51 UTC (permalink / raw) To: zsh-workers; +Cc: 732726 On 2013-12-20 12:27:00 -0800, Bart Schaefer wrote: > I think that's unlikely unless svnwrapper is using zpty. I'm not a user > of SVN at this point so I don't have a reference. That's the pipe itself that is completely broken. The problem can be reproduced with "zsh -f" (version 5.0.3) and: ypig% foo() { ls -R / } ypig% foo | head -- Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-20 23:51 ` Vincent Lefevre @ 2013-12-20 23:59 ` Vincent Lefevre 2013-12-21 0:12 ` Vincent Lefevre 0 siblings, 1 reply; 17+ messages in thread From: Vincent Lefevre @ 2013-12-20 23:59 UTC (permalink / raw) To: zsh-workers On 2013-12-21 00:51:49 +0100, Vincent Lefevre wrote: > The problem can be reproduced with "zsh -f" (version 5.0.3) and: > > ypig% foo() { ls -R / } > ypig% foo | head And also with zsh 5.0.4, which has just been released. -- Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-20 23:59 ` Vincent Lefevre @ 2013-12-21 0:12 ` Vincent Lefevre 2013-12-21 2:19 ` Bart Schaefer 0 siblings, 1 reply; 17+ messages in thread From: Vincent Lefevre @ 2013-12-21 0:12 UTC (permalink / raw) To: zsh-workers A better example: foo() { printf "%d\n" {1..20000} } "foo > /dev/null" terminates immediately. "foo" terminates after a few seconds (output takes a bit time via ssh). "foo | head" freezes. -- Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-21 0:12 ` Vincent Lefevre @ 2013-12-21 2:19 ` Bart Schaefer 2013-12-21 3:42 ` Bart Schaefer 0 siblings, 1 reply; 17+ messages in thread From: Bart Schaefer @ 2013-12-21 2:19 UTC (permalink / raw) To: Vincent Lefevre, zsh-workers On Dec 21, 1:12am, Vincent Lefevre wrote: } Subject: Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function } } A better example: } } foo() { printf "%d\n" {1..20000} } Actually the "ls -R /" example was better because it ruled out problems with SIGPIPE handling and the "print" builtin. This has to be result of either + * 31549: Src/exec,c, Src/zsh.h: replace ad-hoc subsh_close file + descriptor for pipes with new addfilelist() job-based mechanism. (which is the tail of a whole thread starting with 31528) or else it's + * 31919: Src/exec.c, Src/init.c: fix deadlock when a shell builtin + with a multio redirection is used on the left side of a pipeline, + by making sure stdin/out/err file descriptors are closed for the + multio copy process, which means not re-using those descriptors + after they are closed and marked FDT_UNUSED in fdtable[]. For + completeness, initialize their fdtable[] state to FDT_EXTERNAL. because the problem is that the parent shell still holds open the file descriptor that is the standard output of the shell function, so the write is blocked rather than getting SIGPIPE or other failure. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-21 2:19 ` Bart Schaefer @ 2013-12-21 3:42 ` Bart Schaefer 2013-12-21 7:16 ` Bart Schaefer 2013-12-21 18:08 ` Peter Stephenson 0 siblings, 2 replies; 17+ messages in thread From: Bart Schaefer @ 2013-12-21 3:42 UTC (permalink / raw) To: Vincent Lefevre, zsh-workers On Dec 20, 6:19pm, Bart Schaefer wrote: } } + * 31549: Src/exec,c, Src/zsh.h: replace ad-hoc subsh_close file } + descriptor for pipes with new addfilelist() job-based mechanism. Yep, that's the one. I compiled commit 709dbbbda82efde2020d9d67a19687c101b91570 and was able to reproduce the problem. The immediately preceding commit 39ab9952e8255cb99e9c0abcc8bbec43158a55d7 does not show the bug. Non-interactive shells also do not show the bug, which may be why "make check" didn't flag anything. The problem seems to be with this hunk specifically: @@ -1744,8 +1742,6 @@ execpline2(Estate state, wordcode pcode, execpline2(state, *state->pc++, how, pipes[0], output, last1); list_pipe = old_list_pipe; cmdpop(); - zclose(pipes[0]); - subsh_close = -1; } } The problem is that pipes[0] isn't always added to the list of files for the job; sometimes it really does need to be closed there. diff --git a/Src/exec.c b/Src/exec.c index dccdc2b..4480033 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1691,6 +1691,7 @@ execpline2(Estate state, wordcode pcode, execcmd(state, input, output, how, last1 ? 1 : 2); else { int old_list_pipe = list_pipe; + int subsh_close = -1; Wordcode next = state->pc + (*state->pc), pc; wordcode code; @@ -1738,6 +1739,7 @@ execpline2(Estate state, wordcode pcode, } else { /* otherwise just do the pipeline normally. */ addfilelist(NULL, pipes[0]); + subsh_close = pipes[0]; execcmd(state, input, pipes[1], how, 0); } zclose(pipes[1]); @@ -1750,6 +1752,8 @@ execpline2(Estate state, wordcode pcode, execpline2(state, *state->pc++, how, pipes[0], output, last1); list_pipe = old_list_pipe; cmdpop(); + if (subsh_close != pipes[0]) + zclose(pipes[0]); } } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-21 3:42 ` Bart Schaefer @ 2013-12-21 7:16 ` Bart Schaefer 2013-12-21 18:08 ` Peter Stephenson 1 sibling, 0 replies; 17+ messages in thread From: Bart Schaefer @ 2013-12-21 7:16 UTC (permalink / raw) To: zsh-workers On Dec 20, 7:42pm, Bart Schaefer wrote: } } The problem is that pipes[0] isn't always added to the list of files } for the job; sometimes it really does need to be closed there. Here's a regression test. I assume it's OK to do this coproc trick, since if it were not tests A01 and A04 would already have failed. I verified that this test *fails* with 5.0.3 and 5.0.4 (sigh). diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst index c8320a1..61d24fe 100644 --- a/Test/A05execution.ztst +++ b/Test/A05execution.ztst @@ -202,3 +202,15 @@ F:the bug is still there or it reappeared. See workers-29973 for details. 0:Check $pipestatus with a known difficult case >1 0 1 0 0 F:This similar test was triggering a reproducible failure with pipestatus. + + { unsetopt MONITOR } 2>/dev/null + coproc { read -Et 5 || kill -INT $$ } + print -u $ZTST_fd 'This test takes 5 seconds to fail...' + { printf "%d\n" {1..20000} } | ( read -E ) + print -p done + read -Ep +0:Bug regression: piping a shell construct to an external process may hang +>1 +>done +F:This test checks for a file descriptor leak that could cause the left +F:side of a pipe to block on write after the right side has exited ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-21 3:42 ` Bart Schaefer 2013-12-21 7:16 ` Bart Schaefer @ 2013-12-21 18:08 ` Peter Stephenson 2013-12-21 20:57 ` Bart Schaefer 1 sibling, 1 reply; 17+ messages in thread From: Peter Stephenson @ 2013-12-21 18:08 UTC (permalink / raw) To: zsh-workers On Fri, 20 Dec 2013 19:42:23 -0800 Bart Schaefer <schaefer@brasslantern.com> wrote: > The problem is that pipes[0] isn't always added to the list of files > for the job; sometimes it really does need to be closed there. Thanks for the quick work. Are we happy this fixes the particular problem (the regression test looks plausible, don't see why we shouldn't use coproc)? If so I'll make a new version immediately. This evening is my last opportunity for a while so if this is a definite improvement I'd like to get on with it. -- Peter Stephenson <p.w.stephenson@ntlworld.com> Web page now at http://homepage.ntlworld.com/p.w.stephenson/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-21 18:08 ` Peter Stephenson @ 2013-12-21 20:57 ` Bart Schaefer 2013-12-21 22:34 ` Peter Stephenson 0 siblings, 1 reply; 17+ messages in thread From: Bart Schaefer @ 2013-12-21 20:57 UTC (permalink / raw) To: zsh-workers On Dec 21, 6:08pm, Peter Stephenson wrote: } } Thanks for the quick work. The timing was convenient and I was afraid it might have been my fault. } Are we happy this fixes the particular problem (the regression test looks } plausible, don't see why we shouldn't use coproc)? If so I'll make a } new version immediately. As far as I can tell this fixes the problem. It'd be nice to hear from Vincent for confirmation but as that may not happen in time I'd say you should go ahead. -- Barton E. Schaefer ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-21 20:57 ` Bart Schaefer @ 2013-12-21 22:34 ` Peter Stephenson 2013-12-22 1:34 ` Bart Schaefer 0 siblings, 1 reply; 17+ messages in thread From: Peter Stephenson @ 2013-12-21 22:34 UTC (permalink / raw) To: zsh-workers On Sat, 21 Dec 2013 12:57:18 -0800 Bart Schaefer <schaefer@brasslantern.com> wrote: > As far as I can tell this fixes the problem. It'd be nice to hear from > Vincent for confirmation but as that may not happen in time I'd say you > should go ahead. Unfortunately it (the patch in 32171) doesn't fix the problem for me. With foo() { ls -R / } foo | head I'm still getting a hang. I think we're forking inside execcmd() after adding pipes[0] to the filelist for thisjob. This subshell is what's going to form the LHS of the pipeline --- and we entersubsh() which will clear the job table. So I think we need to salvage the filelist from the job table and remove the pipe file descriptors in the danger cases, which I take to be the places where we were handling subsh_close in the old version of the code (where we are handling nested shell constructs of some sort). The following does seem to fix the hang here and not cause any new test failures. Note it includes your code change, but not your regression test (you hadn't pushed either yet last I looked). The change to zwaitjob() is just pulling out the common code shared with the two new cases. I'm not going to commit this and make a new release just on the basis of my guesses, however. diff --git a/Src/exec.c b/Src/exec.c index dccdc2b..f16cfd3 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1691,6 +1691,7 @@ execpline2(Estate state, wordcode pcode, execcmd(state, input, output, how, last1 ? 1 : 2); else { int old_list_pipe = list_pipe; + int subsh_close = -1; Wordcode next = state->pc + (*state->pc), pc; wordcode code; @@ -1738,6 +1739,7 @@ execpline2(Estate state, wordcode pcode, } else { /* otherwise just do the pipeline normally. */ addfilelist(NULL, pipes[0]); + subsh_close = pipes[0]; execcmd(state, input, pipes[1], how, 0); } zclose(pipes[1]); @@ -1750,6 +1752,8 @@ execpline2(Estate state, wordcode pcode, execpline2(state, *state->pc++, how, pipes[0], output, last1); list_pipe = old_list_pipe; cmdpop(); + if (subsh_close != pipes[0]) + zclose(pipes[0]); } } @@ -2385,7 +2389,7 @@ static void execcmd(Estate state, int input, int output, int how, int last1) { HashNode hn = NULL; - LinkList args; + LinkList args, filelist = NULL; LinkNode node; Redir fn; struct multio *mfds[10]; @@ -2907,6 +2911,7 @@ execcmd(Estate state, int input, int output, int how, int last1) flags |= ESUB_KEEPTRAP; if (type == WC_SUBSH && !(how & Z_ASYNC)) flags |= ESUB_JOB_CONTROL; + filelist = jobtab[thisjob].filelist; entersubsh(flags); close(synch[1]); forked = 1; @@ -3260,6 +3265,7 @@ execcmd(Estate state, int input, int output, int how, int last1) if (is_shfunc) { /* It's a shell function */ + pipecleanfilelist(filelist); execshfunc((Shfunc) hn, args); } else { /* It's a builtin */ @@ -3338,6 +3344,7 @@ execcmd(Estate state, int input, int output, int how, int last1) DPUTS(varspc, "BUG: assignment before complex command"); list_pipe = 0; + pipecleanfilelist(filelist); /* If we're forked (and we should be), no need to return */ DPUTS(last1 != 1 && !forked, "BUG: not exiting?"); DPUTS(type != WC_SUBSH, "Not sure what we're doing."); diff --git a/Src/jobs.c b/Src/jobs.c index 371b8eb..a321172 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -1173,6 +1173,30 @@ addfilelist(const char *name, int fd) zaddlinknode(ll, jf); } +/* Clean up pipes no longer needed associated with a job */ + +/**/ +void +pipecleanfilelist(LinkList filelist) +{ + LinkNode node; + + if (!filelist) + return; + node = firstnode(filelist); + while (node) { + Jobfile jf = (Jobfile)getdata(node); + if (jf->is_fd) { + LinkNode next = nextnode(node); + zclose(jf->u.fd); + (void)remnode(filelist, node); + zfree(jf, sizeof(*jf)); + node = next; + } else + incnode(node); + } +} + /* Finished with list of files for a job */ /**/ @@ -1415,19 +1439,7 @@ zwaitjob(int job, int wait_cmd) * we can't deadlock on the fact that those still exist, so * that's not a problem. */ - LinkNode node = firstnode(jn->filelist); - while (node) { - Jobfile jf = (Jobfile)getdata(node); - if (jf->is_fd) { - LinkNode next = nextnode(node); - (void)remnode(jn->filelist, node); - zclose(jf->u.fd); - zfree(jf, sizeof(*jf)); - node = next; - } else { - incnode(node); - } - } + pipecleanfilelist(jn->filelist); } while (!errflag && jn->stat && !(jn->stat & STAT_DONE) && -- Peter Stephenson <p.w.stephenson@ntlworld.com> Web page now at http://homepage.ntlworld.com/p.w.stephenson/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-21 22:34 ` Peter Stephenson @ 2013-12-22 1:34 ` Bart Schaefer 2013-12-23 2:06 ` (potential regression in 5.0.3) Paul Ackersviller 2014-01-02 18:06 ` Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze Peter Stephenson 0 siblings, 2 replies; 17+ messages in thread From: Bart Schaefer @ 2013-12-22 1:34 UTC (permalink / raw) To: zsh-workers On Dec 21, 10:34pm, Peter Stephenson wrote: } } Unfortunately it (the patch in 32171) doesn't fix the problem for me. Drat. } I think we're forking inside execcmd() after adding pipes[0] to the } filelist for thisjob. This subshell is what's going to form the LHS of } the pipeline --- and we entersubsh() which will clear the job table. } So I think we need to salvage the filelist from the job table and remove } the pipe file descriptors in the danger cases, which I take to be the } places where we were handling subsh_close in the old version of the code } (where we are handling nested shell constructs of some sort). I wondered if that would be necessary, but couldn't ever manage to get DPUTS() statements in those two places to print anything, so concluded that the issue was in the place that I did patch. What concerns me is whether we might be closing too many file descriptors if we remove all is_fd entries from filelist at that point, but if that's in the parent that's going to do nothing but wait for the child it should be OK. } The following does seem to fix the hang here and not cause any new test } failures. Note it includes your code change, but not your regression } test (you hadn't pushed either yet last I looked). Argh! Did "git commit" but forgot "git push". Did that now. Here's your new bit of 32175 plus an additional regression test (fails with just 32171 but succeeds after 32175). diff --git a/Src/exec.c b/Src/exec.c index 4480033..f16cfd3 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -2389,7 +2389,7 @@ static void execcmd(Estate state, int input, int output, int how, int last1) { HashNode hn = NULL; - LinkList args; + LinkList args, filelist = NULL; LinkNode node; Redir fn; struct multio *mfds[10]; @@ -2911,6 +2911,7 @@ execcmd(Estate state, int input, int output, int how, int last1) flags |= ESUB_KEEPTRAP; if (type == WC_SUBSH && !(how & Z_ASYNC)) flags |= ESUB_JOB_CONTROL; + filelist = jobtab[thisjob].filelist; entersubsh(flags); close(synch[1]); forked = 1; @@ -3264,6 +3265,7 @@ execcmd(Estate state, int input, int output, int how, int last1) if (is_shfunc) { /* It's a shell function */ + pipecleanfilelist(filelist); execshfunc((Shfunc) hn, args); } else { /* It's a builtin */ @@ -3342,6 +3344,7 @@ execcmd(Estate state, int input, int output, int how, int last1) DPUTS(varspc, "BUG: assignment before complex command"); list_pipe = 0; + pipecleanfilelist(filelist); /* If we're forked (and we should be), no need to return */ DPUTS(last1 != 1 && !forked, "BUG: not exiting?"); DPUTS(type != WC_SUBSH, "Not sure what we're doing."); diff --git a/Src/jobs.c b/Src/jobs.c index 371b8eb..a321172 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -1173,6 +1173,30 @@ addfilelist(const char *name, int fd) zaddlinknode(ll, jf); } +/* Clean up pipes no longer needed associated with a job */ + +/**/ +void +pipecleanfilelist(LinkList filelist) +{ + LinkNode node; + + if (!filelist) + return; + node = firstnode(filelist); + while (node) { + Jobfile jf = (Jobfile)getdata(node); + if (jf->is_fd) { + LinkNode next = nextnode(node); + zclose(jf->u.fd); + (void)remnode(filelist, node); + zfree(jf, sizeof(*jf)); + node = next; + } else + incnode(node); + } +} + /* Finished with list of files for a job */ /**/ @@ -1415,19 +1439,7 @@ zwaitjob(int job, int wait_cmd) * we can't deadlock on the fact that those still exist, so * that's not a problem. */ - LinkNode node = firstnode(jn->filelist); - while (node) { - Jobfile jf = (Jobfile)getdata(node); - if (jf->is_fd) { - LinkNode next = nextnode(node); - (void)remnode(jn->filelist, node); - zclose(jf->u.fd); - zfree(jf, sizeof(*jf)); - node = next; - } else { - incnode(node); - } - } + pipecleanfilelist(jn->filelist); } while (!errflag && jn->stat && !(jn->stat & STAT_DONE) && diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst index 61d24fe..6abfd8b 100644 --- a/Test/A05execution.ztst +++ b/Test/A05execution.ztst @@ -207,10 +207,12 @@ F:This similar test was triggering a reproducible failure with pipestatus. coproc { read -Et 5 || kill -INT $$ } print -u $ZTST_fd 'This test takes 5 seconds to fail...' { printf "%d\n" {1..20000} } | ( read -E ) + hang(){ printf "%d\n" {2..20000} | cat }; hang | ( read -E ) print -p done read -Ep 0:Bug regression: piping a shell construct to an external process may hang >1 +>2 >done F:This test checks for a file descriptor leak that could cause the left F:side of a pipe to block on write after the right side has exited ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: (potential regression in 5.0.3) 2013-12-22 1:34 ` Bart Schaefer @ 2013-12-23 2:06 ` Paul Ackersviller 2013-12-23 5:47 ` Bart Schaefer 2014-01-02 18:06 ` Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze Peter Stephenson 1 sibling, 1 reply; 17+ messages in thread From: Paul Ackersviller @ 2013-12-23 2:06 UTC (permalink / raw) To: zsh-workers I've ran across something else that first looked like a regression, but upon further inspection Test/A05execution.ztst was doing this for a couple of months. Strangely, it goes into an infinite loop, but only when run in the background. It does this for me on both Linux and FreeBSD, starting from the commit c3114a7735c85b79771e08bd156470bde1a36950, but on 5.0.2 too. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: (potential regression in 5.0.3) 2013-12-23 2:06 ` (potential regression in 5.0.3) Paul Ackersviller @ 2013-12-23 5:47 ` Bart Schaefer 0 siblings, 0 replies; 17+ messages in thread From: Bart Schaefer @ 2013-12-23 5:47 UTC (permalink / raw) To: zsh-workers On Dec 23, 2:06am, Paul Ackersviller wrote: } } I've ran across something else that first looked like a regression, } but upon further inspection Test/A05execution.ztst was doing this } for a couple of months. Strangely, it goes into an infinite loop, } but only when run in the background. It does this for me on both } Linux and FreeBSD, starting from the commit } c3114a7735c85b79771e08bd156470bde1a36950, but on 5.0.2 too. It's a known issue that changing the MONITOR option from off to on can cause the acquire_pgrp() routine to go into an infinite loop, depending on the shape of the process tree from the foreground. The jobs.c diff in c3114a7 partly fixed that, but the new A05execution test needs setopt MONITOR to create the conditions it is regressing, so it is still possible for that to trigger a loop. Here's an attempt to resolve the remaining infinite-loop condition: diff --git a/Src/jobs.c b/Src/jobs.c index a321172..8719465 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -2619,6 +2619,7 @@ acquire_pgrp(void) sigset_t blockset, oldset; if ((mypgrp = GETPGRP()) > 0) { + long lastpgrp = mypgrp; sigemptyset(&blockset); sigaddset(&blockset, SIGTTIN); sigaddset(&blockset, SIGTTOU); @@ -2639,6 +2640,9 @@ acquire_pgrp(void) if (read(0, NULL, 0) != 0) {} /* Might generate SIGT* */ signal_block(blockset); mypgrp = GETPGRP(); + if (mypgrp == lastpgrp && !interact) + break; /* Unlikely that pgrp will ever change */ + lastpgrp = mypgrp; } if (mypgrp != mypid) { if (setpgrp(0, 0) == 0) { ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2013-12-22 1:34 ` Bart Schaefer 2013-12-23 2:06 ` (potential regression in 5.0.3) Paul Ackersviller @ 2014-01-02 18:06 ` Peter Stephenson 2014-01-02 20:40 ` Bart Schaefer 1 sibling, 1 reply; 17+ messages in thread From: Peter Stephenson @ 2014-01-02 18:06 UTC (permalink / raw) To: zsh-workers I'm now back and looking through the mail, most of which I've already seen as I managed to get it onto my phone while the network was networking (it was not just that it was 2G, I think the backhaul was a woodpecker with a dodgy beak and a morse key it kept missing). It looks like it was either a good or a bad time for me to be away, depending whether you're me or Bart. As far as I can see most of the stuff has been sorted out, by Bart with assistance. If there's nothing to indicate otherwise over the next couple of days I'll make a 5.0.5 with the fixes in at the weekend. First... On Sat, 21 Dec 2013 17:34:59 -0800 Bart Schaefer <schaefer@brasslantern.com> wrote: > } I think we're forking inside execcmd() after adding pipes[0] to the > } filelist for thisjob. This subshell is what's going to form the LHS of > } the pipeline --- and we entersubsh() which will clear the job table. > } So I think we need to salvage the filelist from the job table and remove > } the pipe file descriptors in the danger cases, which I take to be the > } places where we were handling subsh_close in the old version of the code > } (where we are handling nested shell constructs of some sort). > > I wondered if that would be necessary, but couldn't ever manage to get > DPUTS() statements in those two places to print anything, so concluded > that the issue was in the place that I did patch. > > What concerns me is whether we might be closing too many file descriptors > if we remove all is_fd entries from filelist at that point, but if that's > in the parent that's going to do nothing but wait for the child it should > be OK. Yes, I don't think there should be too much problem with what we *are* doing, and the regression test seems fine. I'm a little bit worried about what we're *not* doing. The closure is tied to thisjob. That's probably better than the previous version where it was simply signalled by a static, neither associated with the current job nor with the call hierarchy, but I wonder if we should be closing even more in the subshell (only); should we, in fact, be closing all f.d.s associated with pipes now that we're no longer in a shell that has any interest in them? Or doesn't this actually cause a problem in practice? (We clearly shouldn't be removing files, the other purpose of the filelist, since that's needed at exactly one point after the final use --- it might be cleaner having a separate list to emphasise the different behaviour of files and f.d.s but it's probably not worth the extra pointer now.) Anyway, that's for future study (FFS, as a colleague slightly ambiguously puts it). pws ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze 2014-01-02 18:06 ` Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze Peter Stephenson @ 2014-01-02 20:40 ` Bart Schaefer 0 siblings, 0 replies; 17+ messages in thread From: Bart Schaefer @ 2014-01-02 20:40 UTC (permalink / raw) To: zsh-workers On Jan 2, 6:06pm, Peter Stephenson wrote: } Subject: Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function } } It looks like it was either a good or a bad time for me to be away, } depending whether you're me or Bart. I was at home with a few days off, so it really wasn't a problem. } If there's nothing to indicate otherwise over the next couple of days } I'll make a 5.0.5 with the fixes in at the weekend. There was one thing I left for your attention: Carl's patch in 32196 for copy-prev-shell-word. } > } So I think we need to salvage the filelist from the job table and remove } > } the pipe file descriptors in the danger cases, which I take to be the } > } places where we were handling subsh_close in the old version of the code } > } (where we are handling nested shell constructs of some sort). [...] } > What concerns me is whether we might be closing too many file descriptors } > if we remove all is_fd entries from filelist at that point } } Yes, I don't think there should be too much problem with what we *are* } doing, and the regression test seems fine. Is it only pipes in that filelist? I was concerned that there might be descriptors for real files there, that shouldn't be closed. But again I think it's OK because in that context all the shell is going to do is wait. } I wonder if we should be closing even more in the subshell (only); } should we, in fact, be closing all f.d.s associated with pipes now } that we're no longer in a shell that has any interest in them? Probably yes, which might in fact fix some of the nits with coprocs (though zsh's coproc doesn't seem to have any nits of that particular flavor that e.g. ksh's does not also have). To do that I think we'd need to start keeping track of what kind of fd is in the filelist, and whether it's read or write, not just that it's a descriptor associated with a particular job. } Or doesn't this actually cause a problem in practice? As you pointed out, we're doing better now than we were, and even the old way it hasn't been especially noticable for many years so far. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-01-02 20:40 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-12-20 19:24 Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze Axel Beckert 2013-12-20 20:27 ` Bart Schaefer 2013-12-20 20:49 ` Axel Beckert 2013-12-20 23:51 ` Vincent Lefevre 2013-12-20 23:59 ` Vincent Lefevre 2013-12-21 0:12 ` Vincent Lefevre 2013-12-21 2:19 ` Bart Schaefer 2013-12-21 3:42 ` Bart Schaefer 2013-12-21 7:16 ` Bart Schaefer 2013-12-21 18:08 ` Peter Stephenson 2013-12-21 20:57 ` Bart Schaefer 2013-12-21 22:34 ` Peter Stephenson 2013-12-22 1:34 ` Bart Schaefer 2013-12-23 2:06 ` (potential regression in 5.0.3) Paul Ackersviller 2013-12-23 5:47 ` Bart Schaefer 2014-01-02 18:06 ` Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze Peter Stephenson 2014-01-02 20:40 ` Bart Schaefer
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ 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).