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

[Eli Zaretskii]

> Cc: address@hidden
> From: Matthias Dahl <address@hidden>
> Date: Mon, 6 Nov 2017 15:15:48 +0100
> 
> 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.

Sorry, I don't understand.  When we call emacs_read in other cases in
that function, the result is an ssize_t, a signed type of the same
width as size_t.  So why would you need to handle overflow, when
reading via emacs_read doesn't?

And even if the result could overflow, we have the likes of
INT_ADD_WRAPV to make handling of that easier and safer.

So I don't see why we should give up in this case.

> 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.

I don't agree with this line of reasoning.  We are not developing a
library whose users will be familiar only with the formal interface
definition and nothing else.  We are talking about code that is being
read, studied, and used by Emacs hack^H^H^H^Hdevelopers all the time.
Having a non-trivial function whose behavior is hard to describe and
remember accurately makes using it error-prone, especially if the
deviant behavior happens only in some corner use case that is hard to
reproduce.  In many cases, the information about these corner cases is
lost soon after the code is introduced, and since we have no formal
requirements for what functions like this one should do, and no good
coverage by the test suite, future changes are impossible to test
reliably, in order to make sure they don't break in those corner
cases.

So the situation where "the function FOO returns the number of bytes
read from the subprocess, except when this and that happens, in which
case it returns just 1" -- such situation should be avoided at all
costs, IME.  And in this case, the cost is actually quite low, unless
I'm missing something.

> 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.

Sorry, you lost me half-way through this description.  I actually
meant to return only the increment of how many bytes were read since
the last call, but I don't see why you'd need more than one counter
that will be reset to zero once its value is consumed.

> > 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?

It's a bug that has been there "forever", and the fix is too risky to
have it in the middle of a pretest.  I don't think we understand well
enough all of its implications, and reading from subprocesses is a
very central and very delicate part of Emacs.  Sorry.

Thanks for working on this.