git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Brian Gernhardt <brian@gernhardtsoftware.com>,
	"git@vger.kernel.org List" <git@vger.kernel.org>
Subject: [PATCH] t0008: avoid SIGPIPE race condition on fifo
Date: Fri, 12 Jul 2013 06:35:23 -0400	[thread overview]
Message-ID: <20130712103522.GA4750@sigill.intra.peff.net> (raw)
In-Reply-To: <7vli5drsbw.fsf@alter.siamese.dyndns.org>

On Thu, Jul 11, 2013 at 09:09:55AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Jul 10, 2013 at 12:36:40PM -0400, Brian Gernhardt wrote:
> >
> >> The newest test in t0008 "streaming support for --stdin", seems to
> >> hang sporadically on my MacBook Pro (running 10.8.4).  The hang seems
> >> to be related to running it in parallel with other tests, as I can
> >> only reliably cause it by running with prove  and -j 3.  However, once
> >> that has hung I am able to semi-reliably have it occur by running the
> >> test separately (with the test hung in the background, using separate
> >> trash directories via the --root option).
> >
> > I can't replicate the hang here (on Linux) doing:
> >
> >   for i in `seq 1 30`; do
> >       ./t0008-ignores.sh --root=/tmp/foo/$i &
> >   done
> 
> It seems to hang on me with bash, but not dash, here.

Thanks, I was able to replicate it with bash, and like Brian, I saw it
hanging in the second read. strace revealed that we were blocked on
open("out").

The patch below should fix it. I'm still not sure why the choice of
shell matters; it may simply be a timing fluke that bash is more likely
to hit for some reason.

-- >8 --
Subject: [PATCH] t0008: avoid SIGPIPE race condition on fifo

To test check-ignore's --stdin feature, we use two fifos to
send and receive data. We carefully keep a descriptor to its
input open so that it does not receive EOF between input
lines. However, we do not do the same for its output. That
means there is a potential race condition in which
check-ignore has opened the output pipe once (when we read
the first line), and then writes the second line before we
have re-opened the pipe.

In that case, check-ignore gets a SIGPIPE and dies. The
outer shell then tries to open the output fifo but blocks
indefinitely, because there is no writer.  We can fix it by
keeping a descriptor open through the whole procedure.

This should also help if check-ignore dies for any other
reason (we would already have opened the fifo and would
therefore not block, but just get EOF on read).

However, we are technically still susceptible to
check-ignore dying early, before we have opened the fifo.
This is an unlikely race and shouldn't generally happen in
practice, though, so we can hopefully ignore it.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0008-ignores.sh | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index a56db80..c29342d 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -697,13 +697,21 @@ test_expect_success PIPE 'streaming support for --stdin' '
 	# shell, and then echo to the fd. We make sure to close it at
 	# the end, so that the subprocess does get EOF and dies
 	# properly.
+	#
+	# Similarly, we must keep "out" open so that check-ignore does
+	# not ever get SIGPIPE trying to write to us. Not only would that
+	# produce incorrect results, but then there would be no writer on the
+	# other end of the pipe, and we would potentially block forever trying
+	# to open it.
 	exec 9>in &&
+	exec 8<out &&
 	test_when_finished "exec 9>&-" &&
+	test_when_finished "exec 8<&-" &&
 	echo >&9 one &&
-	read response <out &&
+	read response <&8 &&
 	echo "$response" | grep "^\.gitignore:1:one	one" &&
 	echo >&9 two &&
-	read response <out &&
+	read response <&8 &&
 	echo "$response" | grep "^::	two"
 '
 
-- 
1.8.3.rc1.30.gff0fb75

  reply	other threads:[~2013-07-12 10:35 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-10 16:36 t0008 hang on streaming test (OS X) Brian Gernhardt
2013-07-10 20:35 ` Antoine Pelisse
2013-07-11 16:14   ` Brian Gernhardt
2013-07-11 13:34 ` Jeff King
2013-07-11 16:09   ` Junio C Hamano
2013-07-12 10:35     ` Jeff King [this message]
2013-07-12 16:23       ` [PATCH] t0008: avoid SIGPIPE race condition on fifo Junio C Hamano
2013-07-12 20:42         ` Jeff King
2013-07-12 16:42       ` Brian Gernhardt
2013-07-11 16:13   ` t0008 hang on streaming test (OS X) Brian Gernhardt

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20130712103522.GA4750@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=brian@gernhardtsoftware.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).