From: Jeff King <peff@peff.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>,
"Git List" <git@vger.kernel.org>
Subject: Re: Is t5516 somehow flakey only on macOS?
Date: Sat, 9 Jan 2021 05:34:05 -0500 [thread overview]
Message-ID: <X/mGnY3wR1fGoxcf@coredump.intra.peff.net> (raw)
In-Reply-To: <CAPig+cQ3U4s0qmzoLL=ZBeSyCZm=QqjWz2P36ZUxMzNAyYn-WQ@mail.gmail.com>
On Sat, Jan 09, 2021 at 04:11:09AM -0500, Eric Sunshine wrote:
> In a successful run, the content of `err` is:
>
> fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886
> fatal: remote error: upload-pack: not our ref
> 64ea4c133d59fa98e86a771eda009872d6ab2886
>
> However, in the unsuccessful run from CI, it is:
>
> fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886
> fatal: unable to write to remote: Broken pipe
>
> Perhaps this means something to someone familiar with this area of the code.
This is the same issue discussed in this sub-thread:
https://lore.kernel.org/git/20190828153944.GA29715@sigill.intra.peff.net/
It's not macOS-specific, but for whatever reason it's much easier to
trigger there. There's a reliable reproduction with a sleep() in:
https://lore.kernel.org/git/20190829220630.GH8571@szeder.dev/
The solution is something like the "after a write() error, pump the read
channel to see if we got an ERR packet" patch Gábor showed in the reply
to that message.
But an interesting thing to note: this particular race only happens with
protocol v0 (and the test is forcing v0, because v2 would not produce a
"not our ref" error in the first place). In general I'd suspect that v2
is a bit less susceptible to these types of races, because it has more
of a request/response flow. But it's still possible (e.g., if the server
side ever aborts before reading the whole request).
For this _particular_ test, since we know that it is testing a v0-only
behavior, we might want to just loosen the test. This goes against the
point of adding it in 014ade7484 (upload-pack: send ERR packet for
non-tip objects, 2019-04-13), but it's the best we can do for now.
Something like this:
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 01004ff680..8726d80250 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1260,7 +1260,20 @@ do
git cat-file commit $SHA1_2 &&
test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \
git fetch ../testrepo/.git $SHA1_3 2>err &&
- test_i18ngrep "remote error:.*not our ref.*$SHA1_3\$" err
+ # ideally we would insist this be on a "remote error:"
+ # line to make sure an ERR packet was sent. But
+ # checking that is racy, because the client may see the
+ # pipe close while they are still writing their "done"
+ # line. So check only that it appears in stderr at all,
+ # which will generally pick up the copy that
+ # upload-pack sends to stderr. This is a hack, because
+ # in the real world the upload-pack stderr would often
+ # not be shown to the client at all (e.g., over git://
+ # or over an ssh channel to a non-vanilla server). So
+ # this is papering over a real racy bug that users
+ # might see in the wild, but this is the best we can do
+ # for now to make our tests robust.
+ test_i18ngrep "not our ref.*$SHA1_3\$" err
)
'
done
-Peff
next prev parent reply other threads:[~2021-01-09 10:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-09 0:37 Is t5516 somehow flakey only on macOS? Junio C Hamano
2021-01-09 9:11 ` Eric Sunshine
2021-01-09 10:34 ` Jeff King [this message]
2021-01-09 10:48 ` Jeff King
2021-01-09 10:57 ` Eric Sunshine
2021-01-09 17:33 ` SZEDER Gábor
2021-01-09 22:42 ` Junio C Hamano
2021-01-10 3:23 ` Jeff King
2021-01-09 22:29 ` Junio C Hamano
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=X/mGnY3wR1fGoxcf@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sunshine@sunshineco.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).