Re: master 8dcb19f 4/4: Add a unit test testing interaction between threads and processes.

[Philipp]

> Am 18.03.2021 um 16:39 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Philipp <p.stephani2@gmail.com>
>> Date: Thu, 18 Mar 2021 13:51:10 +0100
>> Cc: Philipp Stephani <phst@google.com>,
>> emacs-devel@gnu.org
>> 
>>> See below: I don't see how you ensure in this test that each thread
>>> waits for just one process.
>> 
>> Shouldn't providing a process argument to accept-process-output ensure this, 
> 
> No.  All it ensures is that accept-process-output will not return
> until it heard from that process.  It doesn't say what would happen
> with output from other subprocesses that might be running.

Yeah, but why should that matter?  If multiple processes are started in 
parallel, surely people shouldn't assume that there's some deterministic order 
of output arrival across processes.

> 
>> the test should pass whether the process objects have thread affinity or not 
>> (and that's the case for me).
> 
> But the test has no control of the affinity, so it just tests some
> case, not the other(s).  We have no idea what happens in those cases
> it doesn't test.

Sure, that's the nature of unit tests: they only test a single scenario.

> 
>> This might be how the implementation behaves (I haven't checked, but I have 
>> no reason to doubt your explanation).  However, the test doesn't test the 
>> implementation, but the observable behavior.  That is, I can't formally 
>> prove that the implementation leads to the observable behavior that the test 
>> expects, but I think it should, and I'd consider it a bug (in Emacs, not the 
>> test code) if it doesn't.
> 
> It sounds like the "observable behavior" you care about is a very
> small part of what's going on in that case.  As one of many tests in
> this area, that is entirely fine.  But since it's the _only_ test, it
> sounds like we are testing a very small part of the functionality, and
> leave the rest entirely untested.

Again, every unit tests behaves like this, unless you strive for 100% path 
coverage (which is generally infeasible for a very complex function like 
accept-process-output).

I certainly won't stop anyone from adding further tests testing different 
scenarios.

> 
>>> is it important whether a thread exits
>>> because it read output or because it received SIGCHLD and read the
>>> self-pipe?
>> 
>> No, these are implementation details: they might as well not exist as far as 
>> the observable behavior of accept-process-output is concerned.
>> 
>>> is it important whether all the threads exit one after
>>> another, or is it okay if they all wait until one thread exits and
>>> then exit immediately (because all the processes have already died)?
>> 
>> I don't think that's observable/documented behavior: we only know that a 
>> thread has exited after thread-join returns.
> 
> Threads and their interaction with subprocesses were implemented to
> provide some useful behavior which is supposed to support practical
> use cases and applications.  If all we care about is that 10 threads
> don't deadlock and all the processes exited, we didn't even scratch
> the surface of those use cases and applications.  In particular, a
> practical application of these facilities may very well care whether
> the threads exit one by one or not.  It sounds very strange to me to
> hear that we shouldn't care.

The test in question tests what I've described, which is obviously not the only 
thing we should care about.

> 
>>>> I wrote the test based on my understanding of the expected and promised 
>>>> behavior of the process and thread functions, not the Posix implementation.
>>> 
>>> Where and how did you obtain that understanding?  The documentation is
>>> (intentionally or unintentionally) very vague regarding these details.
>> 
>> Yes, so I had to fill in some aspects that I hope are reasonable.  In 
>> particular, the documentation doesn't state explicitly that waiting for a 
>> process in a different thread than the original one is supported, but given 
>> set-process-thread it implicitly has to be, because what would otherwise be 
>> the purpose of set-process-thread?
> 
> If you aren't interested in implementation details, there's no useful
> answer to such questions.  For example, maybe whoever implemented
> set-process-thread had some strange ideas or simply made a design
> mistake?

Or we can check the manual?

"Because threads were a relatively late addition to Emacs Lisp, and due
to the way dynamic binding was sometimes used in conjunction with
‘accept-process-output’, by default a process is locked to the thread
that created it."

That's the given explanation: to prevent bugs when using dynamic binding.  
However, the test in question doesn't use dynamic binding.  We have to call 
set-process-thread in the test because it would fail otherwise, but apart from 
that, the question of thread locking (of affinity) should be unrelated.

> 
>>> Perhaps you only cared about whether the code should or shouldn't err,
>>> but that's clearly not the only interesting or important aspect of
>>> this exercise, is it?
>> 
>> It's at least one important aspect, but finding deadlocks (hangs) is 
>> probably more important.
> 
> I think there are aspects of this much more important than deadlocks
> (or lack thereof).

And you're welcome to add tests for those aspects.

> 
>> What I expect is the behavior of accept-process-output, as explained above. 
>> Specifically: accept-process-output will return nil quickly (within 
>> milliseconds, assuming the calling thread is unblocked) once the given 
>> process has exited.  Once it returns, all process output has been given to 
>> process filters/buffers, and the sentinel has run.
> 
> When the process exits, some thread which waits on its descriptor, or
> on the self-pipe descriptor, will return from the 'pselect' call, and
> will attempt to take the global lock.  If it cannot take the global
> lock, it will block, in which case neither the process output will be
> delivered nor the sentinel will run, until the thread becomes
> unblocked.  So, as you see, even your simple expectations aren't
> really guaranteed.  And your test still succeeds, regardless.  What
> does that tell you about the usefulness of the test?

Nothing.  You seem to have very different ideas about the purpose of unit tests 
than I, so I don't think we should belabor this point any further.

> 
>>>> What I’d assume is that the `thread-join’ calls are synchronization points 
>>>> — after calling `thread-join’ the thread in question is guaranteed to have 
>>>> finished, and since the thread has been waiting for a process to finish 
>>>> (using the accept-process-output loop), the process is also guaranteed to 
>>>> have finished, and this sequence of event is guaranteed exactly in that 
>>>> order.
>>> 
>>> I don't see how you reach those conclusions.  The first part is true:
>>> when thread-join exits, the thread it was waiting for is guaranteed to
>>> have exited.  But how it exited, and what caused it to exit -- these
>>> are open questions, in a situation where 10 subprocesses generate
>>> output and produce 10 signals.
>> 
>> There's only one way for a thread to exit: when its thread function returns. 
> 
> No, a thread can also exit if it signals an error.

Of course, like any function.  I was just being a bit sloppy.  The unit test 
verifies that the threads don't signal errors.

> 
>>> The order of the first two is guaranteed because a single thread
>>> arranges for that sequentially.  But do you care if the sequence is
>>> like this:
>>> 
>>> . all the 10 processes exit
>>> . one of the threads exit because the processes exited
>>> . the other 9 threads get awoken one by one, exit
>>>   accept-process-output because of SIGCHLD, end exit
>> 
>> 
>> From what I can see, that should be fine.  The test only checks that the 
>> threads finish eventually and don't deadlock, but doesn't care about the 
>> order of events across threads (which is not guaranteed unless there are 
>> explicit synchronization points like mutexes or condition variables).
> 
> OK, but then I think a test that is only interested in these
> superficial aspects is not very useful for making sure this part of
> Emacs is really useful in Real Life.  Unless we add other tests which
> do examine what you consider to be "implementation details".

See above.

Since this discussion doesn't seem to lead anywhere, maybe we can just agree to 
disagree and move on?