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

[Philipp <p.stephani2@gmail.com>]

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