git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, szeder.dev@gmail.com, dstolee@microsoft.com,
	gitster@pobox.com
Subject: Re: t5324-split-commit-graph.sh flaky due to assumptions about ulimit behavior
Date: Thu, 24 Jun 2021 11:52:59 -0400	[thread overview]
Message-ID: <YNSqW3G5Sc/gBWAs@coredump.intra.peff.net> (raw)
In-Reply-To: <87im231sj6.fsf@evledraar.gmail.com>

On Thu, Jun 24, 2021 at 11:51:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

> I.e. here we do an "ulimit -n 32" and then run a command, which makes a
> lot of assumptions about how git is compiled, starts up etc, a lot of
> which are outside of our control and up to the OS. It's not 32 files we
> open, but 32 everything. When I could reproduce this it was failing on
> opening some libpcre file or other, so maybe I linked to one too many
> libraries.
> 
> The one other test that uses this pattern seems like it could be
> similarly affected, but I haven't had that fail: d415ad022d8
> (update-ref: test handling large transactions properly, 2015-04-14)
> 
> Since I can't reproduce this anymore maybe I'm reporting a
> nothingburger. I just wonder if this can really work reliably in the
> general case, and whether a reliably version of this pattern doesn't
> need to be one/some of:
> 
>  1. Some GIT_TEST_* mode that sets the (unportable) ulimit itself in the
>     code, after we reach some point. This is how e.g. web-based REPLs
>     often work, load all your known libraries, forbid any file openings
>     (or just N number) after that.
> 
>  2. Ditto, but have the GIT_TEST_* print to stderr if we reach some
>     "checkpoint", have the test only run under limit N if we can reach
>     that point (i.e. binary search or brute force to find the exact N
>     limit).
> 
>  3. Maybe we can assume this would work reliably in cases of a really
>     high limit of N, i.e. the d415ad022d8 test doesn't do this, but my
>     understanding of it is that we're trying to guard against having all
>     loose refs opened at once. So if we create e.g. 2k refs and operate
>     on them we can set the limit to "1999".
> 
>     That's still assuming the same things about ulimit that make/made
>     this test flaky, but we can probably safely assume that just getting
>     to "git <something>" being invoked won't open >1k files, but maybe
>     not >32.

Yes, we could probably just set the "32" a bit higher. Something like
"128" may be more conservative. I'd be hesitant to go as high as
something like 1999; system defaults are often much lower than that
anyway and may get rejected. We may have to adjust the tests to match
the idea of what's "a lot of descriptors". (Likewise, the "ulimit -s"
stuff has to make the same guess).

The prereq also tries running with the lower ulimit, but it only runs
"true". Perhaps running something like "git version" would give us a
more realistic idea of the baseline for running a git command (in which
case the test would be auto-skipped if somehow 32 descriptors isn't
enough on some system). That's not foolproof, though (it might take 31
to run "git version", but 33 to run "update-ref" or something).

I'm willing to let it lie unless you have a current problem. This is all
known to be guesswork/heuristics, and the hope was that it simply
wouldn't come up. If it's causing even occasional pain we might need to
deal with it. But if it's an ephemeral thing that maybe went away, I'm
content to stick my fingers back in my ears. :)

-Peff

  reply	other threads:[~2021-06-24 15:53 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-23 21:40 [PATCH 0/4] commit-graph: handle file descriptor exhaustion Taylor Blau
2020-04-23 21:41 ` [PATCH 1/4] commit-graph.c: don't use discarded graph_name in error Taylor Blau
2020-04-23 21:41 ` [PATCH 2/4] t/test-lib.sh: make ULIMIT_FILE_DESCRIPTORS available to tests Taylor Blau
2020-04-23 21:41 ` [PATCH 3/4] commit-graph.c: gracefully handle file descriptor exhaustion Taylor Blau
2021-06-24  9:51   ` t5324-split-commit-graph.sh flaky due to assumptions about ulimit behavior Ævar Arnfjörð Bjarmason
2021-06-24 15:52     ` Jeff King [this message]
2020-04-23 21:41 ` [PATCH 4/4] commit-graph: close descriptors after mmap Taylor Blau
2020-04-23 22:04   ` Junio C Hamano
2020-04-24  3:56     ` Jeff King
2020-04-24 13:17     ` Derrick Stolee
2020-04-24 16:35       ` Taylor Blau
2020-04-24 20:02       ` Junio C Hamano
2020-04-27 10:57         ` Derrick Stolee
2020-04-23 21:43 ` [PATCH 0/4] commit-graph: handle file descriptor exhaustion Taylor Blau

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=YNSqW3G5Sc/gBWAs@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --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).