Re: [RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?
Junio C Hamano writes:
> That leaves us to something along this line...
>
>> (3) Add a method "test_must_fail_or_die" to
>> "test-lib-functions.sh". This method accepts exit codes 129> too. Use the new method in t5516.
>
> ... but I have to wonder if 129 that the only error we expect, other than the orderly shutdown, is
> reception of sigpipe.
So, how about doing something like this as a starter. All of the
object transport codepath share "we may notice that the other end
disconnected, or that the other end explicitly told us it found an
error, and die, or the other end may have died, perhaps after giving
a human-readable error message, and we end up dying when we try to
tell them more with SIGPIPE", and that by itself is not a bug in the
real life---we will exit with non-zero status and that is a sign
enough for the user to know that the command has failed.
t/test-lib-functions.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6dffb8b..b1f950d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -579,6 +579,9 @@ test_must_fail () {
if test $exit_code = 0; then
echo >&2 "test_must_fail: command succeeded: $*"
return 1
+ elif test $exit_code -eq 141; then
+ echo >&2 "test_must_fail: died with sigpipe: $*"
+ return 0
elif test $exit_code -gt 129 && test $exit_code -le 192; then
echo >&2 "test_must_fail: died by signal: $*"
return 1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?
Lars Schneider writes: > (1) Make upload-pack wait for a response (with timeout) before it > closes the pipe. However, I believe this would not be in line with > the general Git philosophy stated in "git.c" (added in 7559a1be): > "Many parts of Git have subprograms communicate via pipe, expect > the upstream of a pipe to die with SIGPIPE when the downstream of > a pipe does not need to read all that is written." While I think "waiting" with "timeout" is undesirable for the upload-pack / fetch-pack communication, you are rejecting it for a wrong reason. What you quoted is not even a Git philosophy. Yes, many parts do want to behave like that, and the "restore" needs to make sure that such a behaviour happens by fixing the environment given to us by the process spawning us. That does not mean some parts of the system, if they want to run SIGPIPE ignored, must not do so because it goes "against the philosophy". It only means that these parts of the system, if they choose to, must arrange to do so themselves. That "restore" thing is merely to give us a known good state to start from. > (2) Ignore SIGPIPE errors when "fetch-pack" sends a "done" using > "sigchain_push(SIGPIPE, SIG_IGN)" / > "sigchain_pop(SIGPIPE)". However, then the check_pipe function > (added in 756e676c) kicks in and we would need to work around that > as well. I am not sure if we are looking at the issue the right way once we start saying 'do this _when_ fetch sends ...'. The communication between fetch and upload goes bidirectionally in an overlapping fashion, and at any point in the communication, not limited to "after fetch sends 'done'", the other side can decide to disconnect while one side is sending. If you are lucky, you will finish sending the current batch and try to read from the other side and notice "the remote end hung up unexpectedly", and if you are unlucky, you find your write cannot go through due to broken pipe. Dying with SIGPIPE would not give us a chance to clean things up (i.e. react to the "not our ref" error in a more application specific way), so the current behaviour has a room for improvement, but I suspect that changes required for "dying more nicely" would be rather large [*1*]; I am not convinced if it is worth the effort. That leaves us to something along this line... > (3) Add a method "test_must_fail_or_die" to > "test-lib-functions.sh". This method accepts exit codes 129 too. Use the new method in t5516. ... but I have to wonder if 129http://vger.kernel.org/majordomo-info.html
Re: [RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?
Thanks for your explanation Fredrik! However, I believe your 4. step is not what happens in the current implementation as the write call in wrapper.c dies directly. I see three ways to fix the problem: (1) Make upload-pack wait for a response (with timeout) before it closes the pipe. However, I believe this would not be in line with the general Git philosophy stated in "git.c" (added in 7559a1be): "Many parts of Git have subprograms communicate via pipe, expect the upstream of a pipe to die with SIGPIPE when the downstream of a pipe does not need to read all that is written." (2) Ignore SIGPIPE errors when "fetch-pack" sends a "done" using "sigchain_push(SIGPIPE, SIG_IGN)" / "sigchain_pop(SIGPIPE)". However, then the check_pipe function (added in 756e676c) kicks in and we would need to work around that as well. (3) Add a method "test_must_fail_or_die" to "test-lib-functions.sh". This method accepts exit codes 129 wrote: > I think the following happens: > 1. The remote upload-pack finds out "not our ref" > 2. The remote send a response and close the pipe > 3. fetch-pack still tries to write commands to the remote upload-pack > 4. Because the connection has already been closed, writing will fail > with EPIPE which is detected by write_or_die() -> check_pipe() > resulting in die(141) > > The reason for the test to succeed, i.e. git-fetch fails with 128 (or > 1?), in most cases must be because it manages to write everything > before the context switch to the remote upload-pack occurs. > > What is actually the wanted outcome? Should git-fetch try to continue > to see if the already received response is enough to continue as > normal? > > Best regards, > Fredrik > > 2015-10-24 15:08 GMT+02:00 Lars Schneider : >> Hi, >> >> while working on the Git CI integration I noticed that t5516 "75 - deny fetch >> unreachable SHA1, allowtipsha1inwant=true" (introduced in 68ee628) seems to >> be >> flaky on TravisCI. I get the following output in verbose mode: >> > > > >> expecting success: >>mk_empty testrepo && >>( >>cd testrepo && >>git config uploadpack.allowtipsha1inwant >> $configallowtipsha1inwant && >>git commit --allow-empty -m foo && >>git commit --allow-empty -m bar && >>git commit --allow-empty -m xyz >>) && >>SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) && >>SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) && >>SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) && >>( >>cd testrepo && >>git reset --hard $SHA1_2 && >>git cat-file commit $SHA1_1 && >>git cat-file commit $SHA1_3 >>) && >>mk_empty shallow && >>( >>cd shallow && >>test_must_fail git fetch ../testrepo/.git $SHA1_3 && >>test_must_fail git fetch ../testrepo/.git $SHA1_1 && >>git --git-dir=../testrepo/.git config >> uploadpack.allowreachablesha1inwant true && >>git fetch ../testrepo/.git $SHA1_1 && >>git cat-file commit $SHA1_1 && >>test_must_fail git cat-file commit $SHA1_2 && >>git fetch ../testrepo/.git $SHA1_2 && >>git cat-file commit $SHA1_2 && >>test_must_fail git fetch ../testrepo/.git $SHA1_3 >>) >> >> Initialized empty Git repository in >> /home/travis/build/larsxschneider/git/t/trash >> directory.t5516-fetch-push/testrepo/.git/ >> [master (root-commit) a6b22ca] foo >> Author: A U Thor >> [master 5e26403] bar >> Author: A U Thor >> [master 64ea4c1] xyz >> Author: A U Thor >> HEAD is now at 5e26403 bar >> tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 >> author A U Thor 1112912053 -0700 >> committer C O Mitter 1112912053 -0700 >> foo >> tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 >> parent 5e26403b4485d7a44fd8b65dc3f71e21c0dd6fad >> author A U Thor 1112912053 -0700 >> committer C O Mitter 1112912053 -0700 >> xyz >> Initialized empty Git repository in >> /home/travis/build/larsxschneider/git/t/trash >> directory.t5516-fetch-push/shallow/.git/ >> fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886 >> test_must_fail: died by signal: git fetch ../testrepo/.git >> 64ea4c133d59fa98e86a771eda009872d6ab2886 >> not ok 75 - deny fetch unreachable SHA1, allowtipsha1inwant=true >> <<< >> >> "git fetch ../testrepo/.git $SHA1_3" seems to die as follows: >> 1. fetch-pack.c:408 goto done; >> 2. fetch-pack.c:447 done: >> 3. fetch-pack.c:229 static void send_request... (send "0009done\n" --> 9 >> bytes) >> 4. write_or_die.c:74 write_or_die >> 5. wrapper.c:331 writ
Re: [RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?
I think the following happens: 1. The remote upload-pack finds out "not our ref" 2. The remote send a response and close the pipe 3. fetch-pack still tries to write commands to the remote upload-pack 4. Because the connection has already been closed, writing will fail with EPIPE which is detected by write_or_die() -> check_pipe() resulting in die(141) The reason for the test to succeed, i.e. git-fetch fails with 128 (or 1?), in most cases must be because it manages to write everything before the context switch to the remote upload-pack occurs. What is actually the wanted outcome? Should git-fetch try to continue to see if the already received response is enough to continue as normal? Best regards, Fredrik 2015-10-24 15:08 GMT+02:00 Lars Schneider : > Hi, > > while working on the Git CI integration I noticed that t5516 "75 - deny fetch > unreachable SHA1, allowtipsha1inwant=true" (introduced in 68ee628) seems to be > flaky on TravisCI. I get the following output in verbose mode: > > expecting success: > mk_empty testrepo && > ( > cd testrepo && > git config uploadpack.allowtipsha1inwant > $configallowtipsha1inwant && > git commit --allow-empty -m foo && > git commit --allow-empty -m bar && > git commit --allow-empty -m xyz > ) && > SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) && > SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) && > SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) && > ( > cd testrepo && > git reset --hard $SHA1_2 && > git cat-file commit $SHA1_1 && > git cat-file commit $SHA1_3 > ) && > mk_empty shallow && > ( > cd shallow && > test_must_fail git fetch ../testrepo/.git $SHA1_3 && > test_must_fail git fetch ../testrepo/.git $SHA1_1 && > git --git-dir=../testrepo/.git config > uploadpack.allowreachablesha1inwant true && > git fetch ../testrepo/.git $SHA1_1 && > git cat-file commit $SHA1_1 && > test_must_fail git cat-file commit $SHA1_2 && > git fetch ../testrepo/.git $SHA1_2 && > git cat-file commit $SHA1_2 && > test_must_fail git fetch ../testrepo/.git $SHA1_3 > ) > > Initialized empty Git repository in > /home/travis/build/larsxschneider/git/t/trash > directory.t5516-fetch-push/testrepo/.git/ > [master (root-commit) a6b22ca] foo > Author: A U Thor > [master 5e26403] bar > Author: A U Thor > [master 64ea4c1] xyz > Author: A U Thor > HEAD is now at 5e26403 bar > tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 > author A U Thor 1112912053 -0700 > committer C O Mitter 1112912053 -0700 > foo > tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 > parent 5e26403b4485d7a44fd8b65dc3f71e21c0dd6fad > author A U Thor 1112912053 -0700 > committer C O Mitter 1112912053 -0700 > xyz > Initialized empty Git repository in > /home/travis/build/larsxschneider/git/t/trash > directory.t5516-fetch-push/shallow/.git/ > fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886 > test_must_fail: died by signal: git fetch ../testrepo/.git > 64ea4c133d59fa98e86a771eda009872d6ab2886 > not ok 75 - deny fetch unreachable SHA1, allowtipsha1inwant=true > <<< > > "git fetch ../testrepo/.git $SHA1_3" seems to die as follows: > 1. fetch-pack.c:408 goto done; > 2. fetch-pack.c:447 done: > 3. fetch-pack.c:229 static void send_request... (send "0009done\n" --> 9 > bytes) > 4. write_or_die.c:74 write_or_die > 5. wrapper.c:331 write_in_full > 6. wrapper.c:281 xwrite > 7. wrapper.c:287 write --> just dies with exit code 141?! > (use 4688abf to match the line numbers) > > You can find the full logs about the CI run here: > https://travis-ci.org/larsxschneider/git/jobs/86984110 > > I repeatedly executed this test to demonstrate the flakiness: > failed after 100 attempts: > https://travis-ci.org/larsxschneider/git/jobs/87181753 > failed after 1876 attempts: > https://travis-ci.org/larsxschneider/git/jobs/87181754 > (all executed on bbd2929 from https://github.com/larsxschneider/git) > > Unfortunately I was not able to make it fail on my local machine (OS X 10.9) > nor on my VM (Ubuntu 14.10). Travis CI uses Ubuntu 12.04 LTS Server Edition 64 > bit with a AUFS file system. > > Has anyone an idea what might causes these failures or can give me pointers > how > to investigate it? > > Thank you, > Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?
Hi, while working on the Git CI integration I noticed that t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" (introduced in 68ee628) seems to be flaky on TravisCI. I get the following output in verbose mode: >>> expecting success: mk_empty testrepo && ( cd testrepo && git config uploadpack.allowtipsha1inwant $configallowtipsha1inwant && git commit --allow-empty -m foo && git commit --allow-empty -m bar && git commit --allow-empty -m xyz ) && SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) && SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) && SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) && ( cd testrepo && git reset --hard $SHA1_2 && git cat-file commit $SHA1_1 && git cat-file commit $SHA1_3 ) && mk_empty shallow && ( cd shallow && test_must_fail git fetch ../testrepo/.git $SHA1_3 && test_must_fail git fetch ../testrepo/.git $SHA1_1 && git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && git fetch ../testrepo/.git $SHA1_1 && git cat-file commit $SHA1_1 && test_must_fail git cat-file commit $SHA1_2 && git fetch ../testrepo/.git $SHA1_2 && git cat-file commit $SHA1_2 && test_must_fail git fetch ../testrepo/.git $SHA1_3 ) Initialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/testrepo/.git/ [master (root-commit) a6b22ca] foo Author: A U Thor [master 5e26403] bar Author: A U Thor [master 64ea4c1] xyz Author: A U Thor HEAD is now at 5e26403 bar tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 author A U Thor 1112912053 -0700 committer C O Mitter 1112912053 -0700 foo tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 parent 5e26403b4485d7a44fd8b65dc3f71e21c0dd6fad author A U Thor 1112912053 -0700 committer C O Mitter 1112912053 -0700 xyz Initialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/shallow/.git/ fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886 test_must_fail: died by signal: git fetch ../testrepo/.git 64ea4c133d59fa98e86a771eda009872d6ab2886 not ok 75 - deny fetch unreachable SHA1, allowtipsha1inwant=true <<< "git fetch ../testrepo/.git $SHA1_3" seems to die as follows: 1. fetch-pack.c:408 goto done; 2. fetch-pack.c:447 done: 3. fetch-pack.c:229 static void send_request... (send "0009done\n" --> 9 bytes) 4. write_or_die.c:74 write_or_die 5. wrapper.c:331 write_in_full 6. wrapper.c:281 xwrite 7. wrapper.c:287 write --> just dies with exit code 141?! (use 4688abf to match the line numbers) You can find the full logs about the CI run here: https://travis-ci.org/larsxschneider/git/jobs/86984110 I repeatedly executed this test to demonstrate the flakiness: failed after 100 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181753 failed after 1876 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181754 (all executed on bbd2929 from https://github.com/larsxschneider/git) Unfortunately I was not able to make it fail on my local machine (OS X 10.9) nor on my VM (Ubuntu 14.10). Travis CI uses Ubuntu 12.04 LTS Server Edition 64 bit with a AUFS file system. Has anyone an idea what might causes these failures or can give me pointers how to investigate it? Thank you, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
