Re: wait_reading_process_ouput hangs in certain cases (w/ patches)

[Matthias Dahl]

Hello Eli...

On 04/11/17 13:11, Eli Zaretskii wrote:

> OK.  I think I'm okay with your patches, but IMO they need a minor
> improvement: instead of setting got_some_output to an arbitrary value
> of 1, the code should set it to the increment of the bytes read from
> the sub-process.  This is what we do when we actually read from the
> process, and I think this scenario should behave the same.  WDYT?

I know what you mean -- and even though this was bugging me as well when
I implemented it, it is actually on purpose.

First of all, I wanted to make as few assumptions as possible about how
processes are used and be on the conservative/defensive side with this
solution, to make sure not to introduce new bugs or corner-cases.

In this particular case, I wanted to minimize any integer overflow
problems. Granted, depending on how many bits unsigned long has, it is
very (!) unlikely to cause any issues, but otoh that is usually how
bugs get introduced in the first place. So if I had calculated the
difference to set got_some_output properly, I would have had to take
the integer overflow problematic into account.

The current solution has exactly one corner-case which is, imho,
extremely unlikely: You would have to run wait_reading_process_output
for a full wrap-around to exactly the same byte count that it was
started with in the first place. It is safe to assume that will
practically really never happen.

Also, no user of wait_reading_process_output uses the return value
in such a way at all -- which I would also consider a bug since it
is not what is officially documented for the return value in the
first place. It simply says positive, negative or zero... without
stating that it will actually return the bytes read.

So, imho, I would lean toward the current solution since it is as
simple as possible with as few corner-cases as possible. If you
would like to keep the status quo though and to always return how
many bytes were actually read, then I suggest a different approach
altogether. Instead of keeping track how many bytes were read for
the total lifetime of a process, we could track only how many bytes
were read for an in-flight wait. The would require a byte counter
and a counter for how many waits are currently in-flight, so the
last one (or first one) could zero the byte counter again. That
would make things quite a bit more complicated, imho... and without
having a proper gain to show for it.

> With that change, it's okay to commit this to the master branch.

May I suggest, even though it is rather late, to also consider this
for the emacs-26 branch? Since it is not a new feature but a bug fix
to a problem people are actually running into (see Magit), I think it
would be justified. Personally, I have run a patched emacs-26 all the
time here without any problems.

Thanks again for taking the time.

So long,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu