On Tue, Sep 22, 2015 at 11:21 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> On Tue, Sep 22, 2015 at 8:58 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html