From 9658846ce3d379b9ff8010a2ed326fcafc10eb82 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Wed, 24 Feb 2016 02:40:16 -0500
Subject: [PATCH 1/4] write_or_die: handle EPIPE in async threads
When write_or_die() sees EPIPE, it treats it specially by
converting it into a SIGPIPE death. We obviously cannot
ignore it, as the write has failed and the caller expects us
to die. But likewise, we cannot just call die(), because
printing any message at all would be a nuisance during
normal operations.
However, this is a problem if write_or_die() is called from
a thread. Our raised signal ends up killing the whole
process, when logically we just need to kill the thread
(after all, if we are ignoring SIGPIPE, there is good reason
to think that the main thread is expecting to handle it).
Inside an async thread, the die() code already does the
right thing, because we use our custom die_async() routine,
which calls pthread_join(). So ideally we would piggy-back
on that, and simply call:
die_quietly_with_code(141);
or similar. But refactoring the die code to do this is
surprisingly non-trivial. The die_routines themselves handle
both printing and the decision of the exit code. Every one
of them would have to be modified to take new parameters for
the code, and to tell us to be quiet.
Instead, we can just teach write_or_die() to check for the
async case and handle it specially. We do have to build an
interface to abstract the async exit, but it's simple and
self-contained. If we had many call-sites that wanted to do
this die_quietly_with_code(), this approach wouldn't scale
as well, but we don't. This is the only place where do this
weird exit trick.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
run-command.c | 10 ++++++++++
run-command.h | 1 +
write_or_die.c | 4 ++++
3 files changed, 15 insertions(+)
diff --git a/run-command.c b/run-command.c
index 13fa452e8c..3add1d66ac 100644
--- a/run-command.c
+++ b/run-command.c
@@ -633,6 +633,11 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
}
+void NORETURN async_exit(int code)
+{
+ pthread_exit((void *)(intptr_t)code);
+}
+
#else
static struct {
@@ -678,6 +683,11 @@ int in_async(void)
return process_is_async;
}
+void NORETURN async_exit(int code)
+{
+ exit(code);
+}
+
#endif
int start_async(struct async *async)
diff --git a/run-command.h b/run-command.h
index 12bb26c2a6..c0969c7695 100644
--- a/run-command.h
+++ b/run-command.h
@@ -121,5 +121,6 @@ struct async {
int start_async(struct async *async);
int finish_async(struct async *async);
int in_async(void);
+void NORETURN async_exit(int code);
#endif
diff --git a/write_or_die.c b/write_or_die.c
index e7afe7a295..49e80aa222 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,8 +1,12 @@
#include "cache.h"
+#include "run-command.h"
static void check_pipe(int err)
{
if (err == EPIPE) {
+ if (in_async())
+ async_exit(141);
+
signal(SIGPIPE, SIG_DFL);
raise(SIGPIPE);
/* Should never happen, but just in case... */
From 9ff18faf2f8578ed1cbc4376b594fa7ff22c9085 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Wed, 24 Feb 2016 02:44:58 -0500
Subject: [PATCH 2/4] fetch-pack: ignore SIGPIPE in sideband demuxer
If the other side feeds us a bogus pack, index-pack (or
unpack-objects) may die early, before consuming all of its
input. As a result, the sideband demuxer may get SIGPIPE
(racily, depending on whether our data made it into the pipe
buffer or not). If this happens and we are compiled with
pthread support, it will take down the main thread, too.
This isn't the end of the world, as the main process will
just die() anyway when it sees index-pack failed. But it
does mean we don't get a chance to say "fatal: index-pack
failed" or similar. And it also means that we racily fail
t5504, as we sometimes die() and sometimes are killed by
SIGPIPE.
So let's ignore SIGPIPE while demuxing the sideband. We are
already careful to check the return value of write(), so we
won't waste time writing to a broken pipe. The caller will
notice the error return from the async thread, though in
practice we don't even get that far, as we die() as soon as
we see that index-pack failed.
The non-sideband case is already fine; we let index-pack
read straight from the socket, so there is no SIGPIPE at
all. Technically the non-threaded async case is also OK
without this (the forked async process gets SIGPIPE), but
it's not worth distinguishing from the threaded case here.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
fetch-pack.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 01e34b689b..f96f6dfb35 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -15,6 +15,7 @@
#include "version.h"
#include "prio-queue.h"
#include "sha1-array.h"
+#include "sigchain.h"
static int transfer_unpack_limit = -1;
static int fetch_unpack_limit = -1;
@@ -671,9 +672,12 @@ static int everything_local(struct fetch_pack_args *args,
static int sideband_demux(int in, int out, void *data)
{
int *xd = data;
+ int ret;
- int ret = recv_sideband("fetch-pack", xd[0], out);
+ sigchain_push(SIGPIPE, SIG_IGN);
+ ret = recv_sideband("fetch-pack", xd[0], out);
close(out);
+ sigchain_pop(SIGPIPE);
return ret;
}
From f3ed0b372d990eade2e4727f17d9ee40003badb1 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Wed, 24 Feb 2016 02:45:49 -0500
Subject: [PATCH 3/4] test_must_fail: report number of unexpected signal
If a command is marked as test_must_fail but dies with a
signal, we consider that a problem and report the error to
stderr. However, we don't say _which_ signal; knowing that
can make debugging easier. Let's share as much as we know.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/test-lib-functions.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index c64e5a5025..8d99eb303f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -617,7 +617,7 @@ test_must_fail () {
return 0
elif test $exit_code -gt 129 && test $exit_code -le 192
then
- echo >&2 "test_must_fail: died by signal: $*"
+ echo >&2 "test_must_fail: died by signal $(($exit_code - 128)): $*"
return 1
elif test $exit_code -eq 127
then
From 43f3afc6bc6b79715ea46aaf341683cbba6ee965 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Wed, 24 Feb 2016 02:48:36 -0500
Subject: [PATCH 4/4] t5504: handle expected output from SIGPIPE death
Commit 8bf4bec (add "ok=sigpipe" to test_must_fail and use
it to fix flaky tests, 2015-11-27) taught t5504 to handle
"git push" racily exiting with SIGPIPE rather than failing.
However, one of the tests checks the output of the command,
as well. In the SIGPIPE case, we will not have produced any
output. If we want the test to be truly non-flaky, we have
to accept either output.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5504-fetch-receive-strict.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 89224edcc5..a3e12d295a 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -101,7 +101,10 @@ test_expect_success 'push with receive.fsckobjects' '
git config transfer.fsckobjects false
) &&
test_must_fail ok=sigpipe git push --porcelain dst master:refs/heads/test >act &&
- test_cmp exp act
+ {
+ test_cmp exp act ||
+ ! test -s act
+ }
'
test_expect_success 'push with transfer.fsckobjects' '