Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

From: Stefan Beller <sbeller@google.com>
Date: Nov 4, 2015, 21:04 UTC
Message-ID: <CAGZ79kbwJrQ9SrGkJsSx9oUcP98dn9wP=ZvgQLRjmPaZtOzanA@mail.gmail.com>
In-Reply-To: <xmqq8u6da448.fsf@gitster.mtv.corp.google.com>

On Wed, Nov 4, 2015 at 12:42 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> Another approach would be to test if we can set to non blocking and if
>> that is not possible, do not buffer it, but redirect the subcommand
>> directly to stderr of the calling process.
>>
>>     if (set_nonblocking(pp->children[i].process.err) < 0) {
>>         pp->children[i].process.err = 2;
>>         degraded_parallelism = 1;
>>     }
>>
>> and once we observe the degraded_parallelism flag, we can only
>> schedule a maximum of one job at a time, having direct output?
>
> I would even say that on a platform that is _capable_ of setting fd
> non-blocking, we should signal a grave error and die if an attempt
> to do so fails, period.

So more like:

    if (platform_capable_non_blocking_IO())
        set_nonblocking_or_die(&pp->children[i].process.err);
    else
        pp->children[i].process.err = 2; /* ugly intermixed output is possible*/

Show 9 quoted lines

>
> On the other hand, on a platform that is known to be incapable
> (e.g. lacks SETFL or NONBLOCK), we have two options.
>
> 1. If we can arrange to omit the intermediary buffer processing
>    without butchering the flow of the main logic with many
>    #ifdef..#endif, then that would make a lot of sense to do so, and
>    running the processes in parallel with mixed output might be OK.
>    It may not be very nice, but should be an acceptable compromise.

>From what I hear this kind of output is very annoying. (One of the

main complaints of repo users beside missing atomic fetch transactions)

Show 5 quoted lines

>
> 2. If we need to sprinkle conditional compilation all over the place
>    to do so, then I do not think it is worth it.  Instead, we should
>    keep a single code structure, and forbid setting numtasks to more
>    than one, which would also remove the need for nonblock IO.

So additional to the code above, we can add the platform_capable_non_blocking_IO() condition to either the ramp up process, or have a

    if (!platform_capable_non_blocking_IO())
        pp.max_processes = 1;

in the init phase. Then we have only 2 places that deal with the problem, no #ifdefs elsewhere.

Show 7 quoted lines

>
> Either way, bringing "parallelism with sequential output" to
> platforms without nonblock IO can be left for a later day, when we
> find either (1) a good approach that does not require nonblock IO to
> do this, or (2) a good approach to do a nonblock IO on these
> platforms (we know about Windows, but there may be others; I dunno).
>