Re: emacs | Pipeline #8399 has failed for master | ee0e259e

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Michael Albinus]

Hi Robert,

process-tests.el fail regularly on emba.gnu.org. They timeout, see the
last link in this reply.

Maybe it is something similar to what you have fixed for hydra tests, a
while ago?

Could you have a look on this?

Thanks, and best regards, Michael.

> Your pipeline has failed.
>
> Project: emacs ( https://emba.gnu.org/emacs/emacs )
> Branch: master ( https://emba.gnu.org/emacs/emacs/-/commits/master )
>
> Commit: ee0e259e ( https://emba.gnu.org/emacs/emacs/-/commit/ee0e259e5d52749999be329afa9c764a8aff0531 )
> Commit Message: Add some tests for align.el
>
> Commit Author: Lars Ingebrigtsen
>
> Pipeline #8399 ( https://emba.gnu.org/emacs/emacs/-/pipelines/8399 ) triggered by EMBA bot ( https://emba.gnu.org/bot )
> had 1 failed build.
>
> Job #9616 ( https://emba.gnu.org/emacs/emacs/-/jobs/9616/raw )
>
> Stage: test
> Name: test-all

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Robert Pluim]

Michael Albinus <michael.albinus@gmx.de> writes:

> Hi Robert,
>
> process-tests.el fail regularly on emba.gnu.org. They timeout, see the
> last link in this reply.
>
> Maybe it is something similar to what you have fixed for hydra tests, a
> while ago?
>

Youʼre thinking of changes for when thereʼs no network connection?

> Could you have a look on this?
>

Is there a way to get src/process-tests.log? That would at least
narrow it down.

Robert

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Michael Albinus]

Robert Pluim <rpluim@gmail.com> writes:

Hi Robert,

> Is there a way to get src/process-tests.log? That would at least
> narrow it down.

No, the whole CI job runs into a timeout. I have wrapped now all tests
in process-tests.el with a timeout of 60 seconds; hopefully it shows us
the log file in case of.

> Robert

Best regards, Michael.

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Michael Albinus]

Hi Robert,

>> Is there a way to get src/process-tests.log? That would at least
>> narrow it down.
>
> No, the whole CI job runs into a timeout. I have wrapped now all tests
> in process-tests.el with a timeout of 60 seconds; hopefully it shows us
> the log file in case of.

See <https://emba.gnu.org/emacs/emacs/-/jobs/9636/raw>.
process-test-sentinel-sit-for has produced a failure. And according to
the timestamps, process-test-stderr-buffer ran into the timeout.

>> Robert

Best regards, Michael.

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Robert Pluim]

Michael Albinus <michael.albinus@gmx.de> writes:

> Hi Robert,
>
>>> Is there a way to get src/process-tests.log? That would at least
>>> narrow it down.
>>
>> No, the whole CI job runs into a timeout. I have wrapped now all tests
>> in process-tests.el with a timeout of 60 seconds; hopefully it shows us
>> the log file in case of.
>
> See <https://emba.gnu.org/emacs/emacs/-/jobs/9636/raw>.
> process-test-sentinel-sit-for has produced a failure. And according to
> the timestamps, process-test-stderr-buffer ran into the timeout.
>

process-test-sentinel-wait-function-working-p which is used by
process-test-sentinel-sit-for looks racy. Just because the process
sentinel has been called is no guarantee that the process has exited
and had its process-status set correctly, perhaps it should loop on
'process-live-p'

The process-test-stderr-buffer timeout I can't explain: nothing there
can wait longer than 2 seconds (unless itʼs 'make-process' thatʼs
timing out?)

Robert

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Philipp Stephani]

Am Mo., 4. Jan. 2021 um 18:21 Uhr schrieb Robert Pluim <rpluim@gmail.com>:
>
> Michael Albinus <michael.albinus@gmx.de> writes:
>
> > Hi Robert,
> >
> >>> Is there a way to get src/process-tests.log? That would at least
> >>> narrow it down.
> >>
> >> No, the whole CI job runs into a timeout. I have wrapped now all tests
> >> in process-tests.el with a timeout of 60 seconds; hopefully it shows us
> >> the log file in case of.
> >
> > See <https://emba.gnu.org/emacs/emacs/-/jobs/9636/raw>.
> > process-test-sentinel-sit-for has produced a failure. And according to
> > the timestamps, process-test-stderr-buffer ran into the timeout.
> >
>
> process-test-sentinel-wait-function-working-p which is used by
> process-test-sentinel-sit-for looks racy. Just because the process
> sentinel has been called is no guarantee that the process has exited
> and had its process-status set correctly, perhaps it should loop on
> 'process-live-p'

AFAIK the only correct way to wait for a process to exit is (while
(accept-process-output PROC)).

>
> The process-test-stderr-buffer timeout I can't explain: nothing there
> can wait longer than 2 seconds (unless itʼs 'make-process' thatʼs
> timing out?)

make-process can in theory deadlock on BSD-like systems due to a race
between setting the close-on-exec flag and other fork+exec
combinations. That's rather unlikely though.
But maybe there's a race between accept-process-output and calling the
sentinel so that the sentinel is called too early, and then
accept-process-output blocks? AFAIK, when splitting stderr and stdout,
one has to wait separately for the two involved processes:
(while (accept-process-output process))
(while (accept-process-output stderr-process))

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

>> process-test-sentinel-wait-function-working-p which is used by
>> process-test-sentinel-sit-for looks racy. Just because the process
>> sentinel has been called is no guarantee that the process has exited
>> and had its process-status set correctly, perhaps it should loop on
>> 'process-live-p'
>
> AFAIK the only correct way to wait for a process to exit is (while
> (accept-process-output PROC)).

I vaguely remember that Stefan did propose (while (accept-process-output
PROC 0)).

Best regards, Michael.

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Robert Pluim]

Philipp Stephani <p.stephani2@gmail.com> writes:

>> The process-test-stderr-buffer timeout I can't explain: nothing there
>> can wait longer than 2 seconds (unless itʼs 'make-process' thatʼs
>> timing out?)
>
> make-process can in theory deadlock on BSD-like systems due to a race
> between setting the close-on-exec flag and other fork+exec
> combinations. That's rather unlikely though.
> But maybe there's a race between accept-process-output and calling the
> sentinel so that the sentinel is called too early, and then
> accept-process-output blocks? AFAIK, when splitting stderr and stdout,
> one has to wait separately for the two involved processes:
> (while (accept-process-output process))
> (while (accept-process-output stderr-process))

The code is this:

    (while (not (or sentinel-called
		    (> (- (float-time) start-time)
		       process-test-sentinel-wait-timeout)))
      (accept-process-output))

You mean between checking 'sentinel-called' and
'accept-process-output', the sentinel is called, the process exit's and
then 'accept-process-output' hangs? I guess thatʼs possible. We could
add a timeout to accept-process-output to verify.

To your other point: thereʼs only one process involved here, stderr is
being sent to a separate buffer (although using a separate process is
possible as well).

Robert

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Andreas Schwab]

On Jan 05 2021, Robert Pluim wrote:

> The code is this:
>
>     (while (not (or sentinel-called
> 		    (> (- (float-time) start-time)
> 		       process-test-sentinel-wait-timeout)))
>       (accept-process-output))
>
> You mean between checking 'sentinel-called' and
> 'accept-process-output', the sentinel is called, the process exit's and
> then 'accept-process-output' hangs? I guess thatʼs possible.

No, that is not possible, as the process sentinel is only called while
inside accept-process-output.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Philipp Stephani]

Am Di., 5. Jan. 2021 um 08:52 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> >> process-test-sentinel-wait-function-working-p which is used by
> >> process-test-sentinel-sit-for looks racy. Just because the process
> >> sentinel has been called is no guarantee that the process has exited
> >> and had its process-status set correctly, perhaps it should loop on
> >> 'process-live-p'
> >
> > AFAIK the only correct way to wait for a process to exit is (while
> > (accept-process-output PROC)).
>
> I vaguely remember that Stefan did propose (while (accept-process-output
> PROC 0)).

I guess that would work as well, but wouldn't it result in a busy
wait? OTOH, accept-process-output with an infinite timeout should
return immediately if the process has terminated (i.e. Emacs has
processed the SIGCHLD signal).

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

Hi Philipp,

>> >> process-test-sentinel-wait-function-working-p which is used by
>> >> process-test-sentinel-sit-for looks racy. Just because the process
>> >> sentinel has been called is no guarantee that the process has exited
>> >> and had its process-status set correctly, perhaps it should loop on
>> >> 'process-live-p'
>> >
>> > AFAIK the only correct way to wait for a process to exit is (while
>> > (accept-process-output PROC)).
>>
>> I vaguely remember that Stefan did propose (while (accept-process-output
>> PROC 0)).
>
> I guess that would work as well, but wouldn't it result in a busy
> wait?

No. If there's no output, or the connection is closed, it shall return nil.
Well, this is also true for (while (accept-process-output PROC)), but
this could block if the process just waits for something to happen.

Sure, the process shall exit in the given test situation. But we test
*whether* it happens, and shall be prepared that it fails.

(OTOH we have wrapped now the tests with a 60 second timeout; this might
be sufficient.)

Best regards, Michael.

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Philipp Stephani]

Am Mi., 6. Jan. 2021 um 13:15 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> Hi Philipp,
>
> >> >> process-test-sentinel-wait-function-working-p which is used by
> >> >> process-test-sentinel-sit-for looks racy. Just because the process
> >> >> sentinel has been called is no guarantee that the process has exited
> >> >> and had its process-status set correctly, perhaps it should loop on
> >> >> 'process-live-p'
> >> >
> >> > AFAIK the only correct way to wait for a process to exit is (while
> >> > (accept-process-output PROC)).
> >>
> >> I vaguely remember that Stefan did propose (while (accept-process-output
> >> PROC 0)).
> >
> > I guess that would work as well, but wouldn't it result in a busy
> > wait?
>
> No. If there's no output, or the connection is closed, it shall return nil.
> Well, this is also true for (while (accept-process-output PROC)), but
> this could block if the process just waits for something to happen.

You're right in that (while (accept-process-output PROC 0)) doesn't
result in a busy wait, yes. However, since accept-process-output
returns nil in that case, it wouldn't wait at all. But these tests
(e.g. process-tests-stderr-filter) are clearly intended to wait for
process exit; they even contain assertions that the process has
exited.
I'm still convinced that (while (accept-process-output PROC)) is the
only correct way to wait for PROC to finish (see the "Accepting
Output" Info node).

> (OTOH we have wrapped now the tests with a 60 second timeout; this might
> be sufficient.)

We should never hit a timeout in these cases. These test processes
should exit immediately and never hang. Any timeout indicates a bug
either in the tests or in Emacs itself.

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

Hi Philipp,

> You're right in that (while (accept-process-output PROC 0)) doesn't
> result in a busy wait, yes. However, since accept-process-output
> returns nil in that case, it wouldn't wait at all. But these tests
> (e.g. process-tests-stderr-filter) are clearly intended to wait for
> process exit; they even contain assertions that the process has
> exited.
> I'm still convinced that (while (accept-process-output PROC)) is the
> only correct way to wait for PROC to finish (see the "Accepting
> Output" Info node).

I believe, (while (accept-process-output PROC 0)) is the proper way to
read pending output from an exited process. That's all.

Best regards, Michael.

Re: emacs | Pipeline #8399 has failed for master | ee0e259e

[Philipp Stephani]

Am So., 17. Jan. 2021 um 13:42 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> Hi Philipp,
>
> > You're right in that (while (accept-process-output PROC 0)) doesn't
> > result in a busy wait, yes. However, since accept-process-output
> > returns nil in that case, it wouldn't wait at all. But these tests
> > (e.g. process-tests-stderr-filter) are clearly intended to wait for
> > process exit; they even contain assertions that the process has
> > exited.
> > I'm still convinced that (while (accept-process-output PROC)) is the
> > only correct way to wait for PROC to finish (see the "Accepting
> > Output" Info node).
>
> I believe, (while (accept-process-output PROC 0)) is the proper way to
> read pending output from an exited process. That's all.

Yeah, I think you're right. However, if we'd need to wait for the
process anyway, we'd need to evaluate

(while (process-live-p PROC) (accept-process-output))
(while (accept-process-output PROC 0))

and that's more complex and subtler than (while (accept-process-output PROC)).

There's another issue that I haven't had the chance to debug yet: If
you evaluate a form like

(let ((tot-calls 0))
  (dotimes (i 100)
    (let* ((calls 0)
           (a (make-process :name "a"
                            :command (list "bash" "-c" "echo out &&
echo err >&2")
                            :stderr (generate-new-buffer "*stderr*")
                            :sentinel (lambda (_a _b) (cl-incf calls))
                            :noquery t)))
      (while (accept-process-output a))
      (cl-incf tot-calls calls)))
  tot-calls)

then the result will often be significantly less than 100, and I think
Emacs should guarantee that it will be exactly 100 (i.e. that the
sentinel is guaranteed to be called once accept-process-output returns
nil).