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

[Eli Zaretskii]

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

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

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

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

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

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

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

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

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