Re: [PATCHv3 02/13] xread: poll on non blocking fds

From: Stefan Beller <sbeller@google.com>
Date: Sep 22, 2015, 18:41 UTC
Message-ID: <CAGZ79kbWVgWg54rTVSV3MeLHzqC2maJKz14y_tPTdkU5SWtF=w@mail.gmail.com>
In-Reply-To: <xmqqeghq48nl.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 22, 2015 at 11:21 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>>>>>         while (1) {
>>>>>                 nr = read(fd, buf, len);
>>>>> -               if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
>>>>> -                       continue;
>>>>> +               if (nr < 0) {
>>>>> +                       if (errno == EINTR)
>>>>> +                               continue;
>>>>> +                       if (errno == EAGAIN || errno == EWOULDBLOCK) {
>>>>> +                               struct pollfd pfd;
>>>>> +                               int i;
>>>>> +                               pfd.events = POLLIN;
>>>>> +                               pfd.fd = fd;
>>>>> +                               i = poll(&pfd, 1, 100);
>>>>
>>>> Why is this poll() using a timeout? Isn't that still a busy wait of
>>>> sorts (even if less aggressive)?
>>
>> True. Maybe we could have just a warning for now?
>>
>>     if (errno == EAGAIN) {
>>         warning("Using xread with a non blocking fd");
>>         continue; /* preserve previous behavior */
>>     }
>
> It is very likely that you will hit the same EAGAIN immediately by
> continuing and end up spewing tons of the same warning, no?

Right.

>
> We may want to slow down and think before sending a knee-jerk
> response (this applies to both of us).

Earlier this year I was doing lots of work in Gerrit, which somehow prevented crossing emails even though I was knee-jerking all the time. Maybe I picked up habits in Gerrit land which are only positive in Gerrit land, while rather harming in a mailing list base workflow such as here in Git land.

Show 5 quoted lines

>
> I actually think a blocking poll(2) here would not be such a bad
> idea.  It would preserves the previous behaviour for callers who
> unknowingly inherited a O_NONBLOCK file descriptor from its
> environment without forcing them to waste CPU cycles.

So rather a combination of both, with the warning only spewing every 5 seconds or such?

I mean we identified this as a potential bug which we want to fix eventually by having the caller make sure they only pass in blocking fds.

I feel like this is similar to the && call chain detection Jeff made a few months back. At first there were bugreports coming in left and right about broken test scripts, now it's all fixed hopefully. The difference is this is not in the test suite though, so maybe we can rollout this patch early next cycle and get all the bugs fixed before we get serious with a release again?

It's risky though. Maybe there is a legit use case for "I have a non blocking fd for $REASONS and I know it, but I still want to read it now until it's done" Actually I thought about doing that exactly myself as part of the cleanup in the asynchronous paralllel processing. We'd collect progress information using this strbuf_read_once and polling. Once a process is done, we would need to get all its output (i.e. up to EOF), so then we could just call strbuf_read as we know the child has already terminated, so all we want is getting the last bytes out of the pipe.