Re: [RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2015, 17:38 UTC
Message-ID: <xmqq8u6qluh2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CFEB6E3F-48A0-41D8-A8FD-D48B806461DB@gmail.com>

Lars Schneider <larsxschneider@gmail.com> writes:

Show 6 quoted lines

> (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.

Show 5 quoted lines

> (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<x<192,
> too. Use the new method in t5516.

... but I have to wonder if 129<x<192 is loosening too much, given that the only error we expect, other than the orderly shutdown, is reception of sigpipe.

[Footnote]

*1* We'd need to update fetch-pack.c:send_request() so that it and
    its helpers use a variant of write() that does not die with
    SIGPIPE and instead returns an error status to the caller, and
    update all the callers to react to an error, which may involve
    jumping to the receive codepath with the hope that some early
    response is already waiting for us to read and gives us an error
    message from the remote side, or something along that line.  And
    we'd probably need to update the other direction, i.e. push and
    receive, for symmetry.