From: Jeff King <peff@peff.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load
Date: Thu, 6 Dec 2018 20:14:25 -0500 [thread overview]
Message-ID: <20181207011425.GB21992@sigill.intra.peff.net> (raw)
In-Reply-To: <20181206231005.GP30222@szeder.dev>
On Fri, Dec 07, 2018 at 12:10:05AM +0100, SZEDER Gábor wrote:
> On Wed, Dec 05, 2018 at 04:56:21PM -0500, Jeff King wrote:
> > Could we just kill them all?
> >
> > I guess it's a little tricky, because $! is going to give us the pid of
> > each subshell. We actually want to kill the test sub-process. That takes
> > a few contortions, but the output is nice (every sub-job immediately
> > says "ABORTED ...", and the final process does not exit until the whole
> > tree is done):
>
> It's trickier than that:
>
> - Nowadays our test lib translates SIGINT to exit, but not TERM (or
> HUP, in case a user decides to close the terminal window), which
> means that if the test runs any daemons in the background, then
> those won't be killed when the stress test is aborted.
>
> This is easy enough to address, and I've been meaning to do this
> in an other patch series anyway.
Yeah, trapping on more signals seems reasonable (or just propagating INT
instead of TERM). I'm more worried about this one:
> - With the (implied) '--verbose-log' the process killed in the
> background subshell's SIGTERM handler is not the actual test
> process, because '--verbose-log' runs the test again in a piped
> subshell to save its output:
Hmm, yeah. The patch I sent earlier already propagates through the
subshell, so this is really just another layer there. I.e., would it be
reasonable when we are using --verbose-log (or --tee) for the parent
process to propagate signals down to child it spawned?
That would fix this case, and it would make things more predictable in
general (e.g., a user who manually runs kill).
It does feel like a lot of boilerplate to be propagating these signals
manually. I think the "right" Unix-y solution here is process groups:
each sub-job should get its own group, and then the top-level runner
script can kill the whole group. We may run into portability issues,
though (setsid is in util-linux, but I don't know if there's an
equivalent elsewhere; a small perl or C helper could do it, but I have
no idea what that would mean on Windows).
> (GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
> echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
>
> That 'kill' kills the "outer" shell process.
> And though I get "ABORTED..." immediately and I get back my
> prompt right away, the commands involved in the above construct
> are still running in the background:
>
> $ ./t3903-stash.sh --stress=1
> ^CABORTED 0.0
> $ ps a |grep t3903
> 1280 pts/17 S 0:00 /bin/sh ./t3903-stash.sh --stress=1
> 1281 pts/17 S 0:00 tee -a <...>/test-results/t3903-stash.stress-0.out
> 1282 pts/17 S 0:00 /bin/sh ./t3903-stash.sh --stress=1
> 4173 pts/17 S+ 0:00 grep t3903
>
> I see this behavior with all shells I tried.
> I haven't yet started to think it through why this happens :)
I think you get ABORTED because we are just watching for the
parent-process to exit (and reporting its status). The fact that that
the subshell (and the tee) are still running is not important. That all
makes sense to me.
> Not implying '--verbose-log' but redirecting the test script's
> output, like you did in your 'stress' script, seems to work in
> dash, ksh, and ks93, but not in Bash v4.3 or later, where, for
> whatever reason, the subshell get SIGINT before the SIGTERM would
> arrive.
> While we could easily handle SIGINT in the subshell with the same
> signal handler as SIGTERM, it bothers me that something
> fundamental is wrong here.
Yeah, I don't understand the SIGINT behavior you're seeing with bash. I
can't replicate it with bash 4.4.23 (from Debian unstable).
-Peff
next prev parent reply other threads:[~2018-12-07 1:14 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-04 16:34 [RFC PATCH 0/3] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
2018-12-04 16:34 ` [PATCH 1/3] test-lib: consolidate naming of test-results paths SZEDER Gábor
2018-12-05 4:57 ` Jeff King
2018-12-04 16:34 ` [PATCH 2/3] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
2018-12-05 5:17 ` Jeff King
2018-12-05 12:20 ` SZEDER Gábor
2018-12-05 21:59 ` Jeff King
2018-12-04 16:34 ` [RFC PATCH 3/3] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
2018-12-04 17:04 ` Ævar Arnfjörð Bjarmason
2018-12-04 17:37 ` SZEDER Gábor
2018-12-05 5:46 ` Jeff King
2018-12-04 18:11 ` Ævar Arnfjörð Bjarmason
2018-12-05 5:50 ` Jeff King
2018-12-05 12:07 ` SZEDER Gábor
2018-12-05 14:01 ` Ævar Arnfjörð Bjarmason
2018-12-05 14:39 ` SZEDER Gábor
2018-12-05 19:59 ` Ævar Arnfjörð Bjarmason
2018-12-05 5:44 ` Jeff King
2018-12-05 10:34 ` SZEDER Gábor
2018-12-05 21:36 ` Jeff King
2018-12-06 0:22 ` Junio C Hamano
2018-12-06 5:35 ` Jeff King
2018-12-06 6:41 ` Junio C Hamano
2018-12-06 22:56 ` SZEDER Gábor
2018-12-07 1:03 ` Jeff King
2018-12-05 14:01 ` SZEDER Gábor
2018-12-05 21:56 ` Jeff King
2018-12-06 23:10 ` SZEDER Gábor
2018-12-07 1:14 ` Jeff King [this message]
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
2018-12-11 10:57 ` Jeff King
2018-12-09 22:56 ` [PATCH v2 2/7] test-lib: parse some --options earlier SZEDER Gábor
2018-12-11 11:09 ` Jeff King
2018-12-11 12:42 ` SZEDER Gábor
2018-12-17 21:44 ` Jeff King
2018-12-30 19:04 ` SZEDER Gábor
2019-01-03 4:53 ` Jeff King
2018-12-09 22:56 ` [PATCH v2 3/7] test-lib: consolidate naming of test-results paths SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 4/7] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 5/7] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 6/7] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 7/7] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
2018-12-10 1:34 ` [PATCH] fixup! " SZEDER Gábor
2018-12-11 11:16 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests Jeff King
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 2/8] test-lib: parse options in a for loop to keep $@ intact SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 3/8] test-lib: parse command line options earlier SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 4/8] test-lib: consolidate naming of test-results paths SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 5/8] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
2018-12-30 22:44 ` SZEDER Gábor
2018-12-30 22:48 ` [PATCH v3.1 " SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 6/8] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
2018-12-31 17:14 ` Carlo Arenas
2018-12-30 19:16 ` [PATCH v3 7/8] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 8/8] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 2/8] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 3/8] test-lib: parse options in a for loop to keep $@ intact SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 4/8] test-lib: parse command line options earlier SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 5/8] test-lib: consolidate naming of test-results paths SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 6/8] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 7/8] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 8/8] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
2019-01-07 8:49 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests Jeff King
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=20181207011425.GB21992@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=szeder.dev@gmail.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).