Re: [PATCH] test: accept death by SIGPIPE as a valid failure mode
Lars Schneider writes: > Oh, I missed this email thread. I am still working on a stable > Travis-CI integration and I ran into this issue a few times. I > fixed it in my (not yet published) patch with an additional > function "test_must_fail_or_sigpipe" that I've used for all tests > affected by this issue. Modifying the "test_must_fail" function > seemed too risky for me as I don't understand all possible > implications. However, if you don't see a problem then this is > fine with me. It's not that I don't see a problem at all. You constructed a good summary of the issues in three bullet points, that lead me to think that it is the right approach to tweak the way the tests evaluate the outcome, but then nothing came out of the discussion, so I sent out a "how about doing it this way" to make sure this topic will not be forgotten. There is nothing more to it, and "how about..." is in no way final. There obviously are pros and cons between introducing your new helper to mark the ones that are allowed to catch SIGPIPE and changing all occurrences of test_must_fail. I do not have a strong opinion yet, but it needs to be discussed and decided. -- 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: [PATCH] test: accept death by SIGPIPE as a valid failure mode
> On 05 Nov 2015, at 08:47, Jeff King wrote: > > On Fri, Oct 30, 2015 at 02:22:14PM -0700, Junio C Hamano wrote: > >> On a local host, the object/history transport code often talks over >> pipe with the other side. The other side may notice some (expected) >> failure, send the error message either to our process or to the >> standard error and hung up. In such codepaths, if timing were not >> unfortunate, our side would receive the report of (expected) failure >> from the other side over the pipe and die(). Otherwise, our side >> may still be trying to talk to it and would die with a SIGPIPE. >> >> This was observed as an intermittent breakage in t5516 by a few >> people. >> >> In the real-life scenario, either mode of death exits with a >> non-zero status, and the user would learn that the command failed. >> The test_must_fail helper should also know that dying with SIGPIPE >> is one of the valid failure modes when we are expecting the tested >> operation to notice problem and fail. > > Sorry for the slow review; before commenting I wanted to dig into > whether this SIGPIPE ambiguity was avoidable in the first place. > > I think the answer is "probably not". We do call write_or_die() pretty > consistently in the network-aware programs. So we could ignore SIGPIPE, > and then we would catch EPIPE (of course, we convert that into SIGPIPE > in many places, but we do not have to do so). But since the SIGPIPE > behavior is global, that carries the risk of us failing to check a write > against some other descriptor. It's probably not worth it. > > Teaching the tests to handle both cases seems like a reasonable > workaround. Changing test_must_fail covers a lot of cases; I wondered if > there are other tests that would not want to silently cover up a SIGPIPE > death. But I could not really think of a plausible reason. > > So I think your patch is the best thing to do. > > -Peff Oh, I missed this email thread. I am still working on a stable Travis-CI integration and I ran into this issue a few times. I fixed it in my (not yet published) patch with an additional function "test_must_fail_or_sigpipe" that I've used for all tests affected by this issue. Modifying the "test_must_fail" function seemed too risky for me as I don't understand all possible implications. However, if you don't see a problem then this is fine with me. - 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
Re: [PATCH] test: accept death by SIGPIPE as a valid failure mode
On Fri, Oct 30, 2015 at 02:22:14PM -0700, Junio C Hamano wrote: > On a local host, the object/history transport code often talks over > pipe with the other side. The other side may notice some (expected) > failure, send the error message either to our process or to the > standard error and hung up. In such codepaths, if timing were not > unfortunate, our side would receive the report of (expected) failure > from the other side over the pipe and die(). Otherwise, our side > may still be trying to talk to it and would die with a SIGPIPE. > > This was observed as an intermittent breakage in t5516 by a few > people. > > In the real-life scenario, either mode of death exits with a > non-zero status, and the user would learn that the command failed. > The test_must_fail helper should also know that dying with SIGPIPE > is one of the valid failure modes when we are expecting the tested > operation to notice problem and fail. Sorry for the slow review; before commenting I wanted to dig into whether this SIGPIPE ambiguity was avoidable in the first place. I think the answer is "probably not". We do call write_or_die() pretty consistently in the network-aware programs. So we could ignore SIGPIPE, and then we would catch EPIPE (of course, we convert that into SIGPIPE in many places, but we do not have to do so). But since the SIGPIPE behavior is global, that carries the risk of us failing to check a write against some other descriptor. It's probably not worth it. Teaching the tests to handle both cases seems like a reasonable workaround. Changing test_must_fail covers a lot of cases; I wondered if there are other tests that would not want to silently cover up a SIGPIPE death. But I could not really think of a plausible reason. So I think your patch is the best thing to do. -Peff -- 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
