From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29866 invoked by alias); 22 Dec 2013 01:35:00 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 32176 Received: (qmail 2932 invoked from network); 22 Dec 2013 01:34:54 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 From: Bart Schaefer Message-id: <131221173459.ZM1039@torch.brasslantern.com> Date: Sat, 21 Dec 2013 17:34:59 -0800 In-reply-to: <20131221223424.6b7a55a7@pws-pc.ntlworld.com> Comments: In reply to Peter Stephenson "Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze" (Dec 21, 10:34pm) References: <20131220192435.GE27889@sym.noone.org> <131220122701.ZM15525@torch.brasslantern.com> <20131220235149.GA21721@xvii.vinc17.org> <20131220235955.GB21721@xvii.vinc17.org> <20131221001235.GC21721@xvii.vinc17.org> <131220181950.ZM15385@torch.brasslantern.com> <131220194223.ZM29152@torch.brasslantern.com> <20131221180846.78a5a013@pws-pc.ntlworld.com> <131221125718.ZM24141@torch.brasslantern.com> <20131221223424.6b7a55a7@pws-pc.ntlworld.com> X-Mailer: OpenZMail Classic (0.9.2 24April2005) To: zsh-workers@zsh.org Subject: Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze MIME-version: 1.0 Content-type: text/plain; charset=us-ascii 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