bug#141: Process sentinel not called in batch mode

bug#141: Process sentinel not called in batch mode

[Chong Yidong]

Helmut Eller <eller.helmut@gmail.com> wrote:

> process-sentinels aren't called in batch mode.  I'm not sure if this
> is a bug or the expected behavior, but I find this quite frustrating.
> To reproduce the problem do:
> $shell> cat test-sentinel.el

  (defvar sentinel-called nil)
  (defun main ()
    (let ((proc (start-process "test" nil "bash" "-c" "sleep 1 ; exit 20")))
      (set-process-sentinel proc (lambda (proc msg)
                                 (message "sentinel: %S %s" proc msg)
                                 (setq sentinel-called t)))
    (while (not sentinel-called)
      (message "status: %s exit-status: %s sentinel-called: %s" 
               (process-status proc) (process-exit-status proc) 
               sentinel-called)
      (sit-for 1)
      (accept-process-output proc 1))))
  (main)

> $shell> emacs -Q -batch -l test-sentinel.el

This problem occurs because wait_reading_process_output only calls
status_notify when given a nonzero DO_DISPLAY argument (process.c:4451).
Although wait_reading_process_output is called from many places in
Emacs, it is given a nonzero DO_DISPLAY argument only during
kbd_buffer_get_event.  But that function is never called in batch mode.

Maybe something like the attached patch should be applied.
Alternatively, we could change Faccept_process_output so that when Emacs
is in batch mode, it calls wait_reading_process_output with a nonzero
DO_DISPLAY argument.  Opinions?

*** trunk/src/process.c.~1.539.~	2008-04-09 13:46:04.000000000 -0400
--- trunk/src/process.c	2008-05-13 19:27:01.000000000 -0400
***************
*** 4414,4420 ****
  	 available, notify the user of the change right away.  After
  	 this explicit check, we'll let the SIGCHLD handler zap
  	 timeout to get our attention.  */
!       if (update_tick != process_tick && do_display)
  	{
  	  SELECT_TYPE Atemp;
  #ifdef NON_BLOCKING_CONNECT
--- 4414,4421 ----
  	 available, notify the user of the change right away.  After
  	 this explicit check, we'll let the SIGCHLD handler zap
  	 timeout to get our attention.  */
!       if (update_tick != process_tick
! 	  && (do_display || noninteractive))
  	{
  	  SELECT_TYPE Atemp;
  #ifdef NON_BLOCKING_CONNECT

bug#141: Process sentinel not called in batch mode

[Stefan Monnier]

> This problem occurs because wait_reading_process_output only calls
> status_notify when given a nonzero DO_DISPLAY argument (process.c:4451).
> Although wait_reading_process_output is called from many places in
> Emacs, it is given a nonzero DO_DISPLAY argument only during
> kbd_buffer_get_event.  But that function is never called in batch mode.

Does anyone know why we test do_display there?


        Stefan

Re: bug#141: Process sentinel not called in batch mode

[Richard M Stallman]

    > This problem occurs because wait_reading_process_output only calls
    > status_notify when given a nonzero DO_DISPLAY argument (process.c:4451).
    > Although wait_reading_process_output is called from many places in
    > Emacs, it is given a nonzero DO_DISPLAY argument only during
    > kbd_buffer_get_event.  But that function is never called in batch mode.

    Does anyone know why we test do_display there?

I think the idea is, if you are going to update the display, it is
good to display the fact that a process has terminated.

I don't claim this implies the code should not be changed.

Re: bug#141: Process sentinel not called in batch mode

[Stefan Monnier]

>> This problem occurs because wait_reading_process_output only calls
>> status_notify when given a nonzero DO_DISPLAY argument (process.c:4451).
>> Although wait_reading_process_output is called from many places in
>> Emacs, it is given a nonzero DO_DISPLAY argument only during
>> kbd_buffer_get_event.  But that function is never called in batch mode.

>     Does anyone know why we test do_display there?

> I think the idea is, if you are going to update the display, it is
> good to display the fact that a process has terminated.

This side I understand, but what about the other: why should we not
check process status in the case where we are not redisplaying?


        Stefan

Re: bug#141: Process sentinel not called in batch mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 16 May 2008 09:25:54 -0400
> Cc: 141@emacsbugs.donarmstrong.com, cyd@stupidchicken.com,
> 	eller.helmut@gmail.com, emacs-devel@gnu.org,
> 	bug-submit-list@donarmstrong.com, bug-gnu-emacs@gnu.org
> 
> >> This problem occurs because wait_reading_process_output only calls
> >> status_notify when given a nonzero DO_DISPLAY argument (process.c:4451).
> >> Although wait_reading_process_output is called from many places in
> >> Emacs, it is given a nonzero DO_DISPLAY argument only during
> >> kbd_buffer_get_event.  But that function is never called in batch mode.
> 
> >     Does anyone know why we test do_display there?
> 
> > I think the idea is, if you are going to update the display, it is
> > good to display the fact that a process has terminated.
> 
> This side I understand, but what about the other: why should we not
> check process status in the case where we are not redisplaying?

Because Emacs generally looks at process output only when it's idle?

Re: bug#141: Process sentinel not called in batch mode

[Stefan Monnier]

>> >> This problem occurs because wait_reading_process_output only calls
>> >> status_notify when given a nonzero DO_DISPLAY argument (process.c:4451).
>> >> Although wait_reading_process_output is called from many places in
>> >> Emacs, it is given a nonzero DO_DISPLAY argument only during
>> >> kbd_buffer_get_event.  But that function is never called in batch mode.
>> 
>> >     Does anyone know why we test do_display there?
>> 
>> > I think the idea is, if you are going to update the display, it is
>> > good to display the fact that a process has terminated.
>> 
>> This side I understand, but what about the other: why should we not
>> check process status in the case where we are not redisplaying?

> Because Emacs generally looks at process output only when it's idle?

I don't follow: we're talking about when should
wait_reading_process_output check status_notify.

So to repeat/rephrase my question: why doesn't
wait_reading_process_output call status_notify when it is passed a zero
do_display?


        Stefan

Re: bug#141: Process sentinel not called in batch mode

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So to repeat/rephrase my question: why doesn't
> wait_reading_process_output call status_notify when it is passed a
> zero do_display?

I don't see any good reason, but the current behavior seems to work OK
for !noninteractive.

Re: bug#141: Process sentinel not called in batch mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rms@gnu.org,  141@emacsbugs.donarmstrong.com,  cyd@stupidchicken.com,  eller.helmut@gmail.com,  emacs-devel@gnu.org,  bug-submit-list@donarmstrong.com,  bug-gnu-emacs@gnu.org
> Date: Fri, 16 May 2008 12:57:16 -0400
> 
> >> This side I understand, but what about the other: why should we not
> >> check process status in the case where we are not redisplaying?
> 
> > Because Emacs generally looks at process output only when it's idle?
> 
> I don't follow: we're talking about when should
> wait_reading_process_output check status_notify.
> 
> So to repeat/rephrase my question: why doesn't
> wait_reading_process_output call status_notify when it is passed a zero
> do_display?

Sorry, let me explain my line of thought in more detail: Emacs only
does a redisplay when it is idle.  Emacs also checks input from
subprocesses when it is idle.  If we are not in redisplay, it means
Emacs is not idle, and so it makes sense not to check for subprocess
output at that time.

Does this make any sense?

Re: bug#141: Process sentinel not called in batch mode

[Stefan Monnier]

>> >> This side I understand, but what about the other: why should we not
>> >> check process status in the case where we are not redisplaying?
>> 
>> > Because Emacs generally looks at process output only when it's idle?
>> 
>> I don't follow: we're talking about when should
>> wait_reading_process_output check status_notify.
>> 
>> So to repeat/rephrase my question: why doesn't
>> wait_reading_process_output call status_notify when it is passed a zero
>> do_display?

> Sorry, let me explain my line of thought in more detail: Emacs only
> does a redisplay when it is idle.  Emacs also checks input from
> subprocesses when it is idle.  If we are not in redisplay, it means
> Emacs is not idle, and so it makes sense not to check for subprocess
> output at that time.

> Does this make any sense?

But if we're in wait_reading_process_output, that means that idle or
not, we're checking for process output, so why not check for process
status changes as well?


        Stefan

Re: bug#141: Process sentinel not called in batch mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rms@gnu.org,  141@emacsbugs.donarmstrong.com,  cyd@stupidchicken.com,  eller.helmut@gmail.com,  emacs-devel@gnu.org,  bug-submit-list@donarmstrong.com,  bug-gnu-emacs@gnu.org
> Date: Fri, 16 May 2008 15:24:04 -0400
> 
> >> >> This side I understand, but what about the other: why should we not
> >> >> check process status in the case where we are not redisplaying?
> >> 
> >> > Because Emacs generally looks at process output only when it's idle?
> >> 
> >> I don't follow: we're talking about when should
> >> wait_reading_process_output check status_notify.
> >> 
> >> So to repeat/rephrase my question: why doesn't
> >> wait_reading_process_output call status_notify when it is passed a zero
> >> do_display?
> 
> > Sorry, let me explain my line of thought in more detail: Emacs only
> > does a redisplay when it is idle.  Emacs also checks input from
> > subprocesses when it is idle.  If we are not in redisplay, it means
> > Emacs is not idle, and so it makes sense not to check for subprocess
> > output at that time.
> 
> > Does this make any sense?
> 
> But if we're in wait_reading_process_output, that means that idle or
> not, we're checking for process output, so why not check for process
> status changes as well?

Because status_notify has a side effect of triggering redisplay.

Re: bug#141: Process sentinel not called in batch mode

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> But if we're in wait_reading_process_output, that means that idle or
> not, we're checking for process output, so why not check for process
> status changes as well?

Because we want to have all process output processed before registering
that the process has finished?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: bug#141: Process sentinel not called in batch mode

[Stefan Monnier]

>> >> >> This side I understand, but what about the other: why should we not
>> >> >> check process status in the case where we are not redisplaying?
>> >> 
>> >> > Because Emacs generally looks at process output only when it's idle?
>> >> 
>> >> I don't follow: we're talking about when should
>> >> wait_reading_process_output check status_notify.
>> >> 
>> >> So to repeat/rephrase my question: why doesn't
>> >> wait_reading_process_output call status_notify when it is passed a zero
>> >> do_display?
>> 
>> > Sorry, let me explain my line of thought in more detail: Emacs only
>> > does a redisplay when it is idle.  Emacs also checks input from
>> > subprocesses when it is idle.  If we are not in redisplay, it means
>> > Emacs is not idle, and so it makes sense not to check for subprocess
>> > output at that time.
>> 
>> > Does this make any sense?
>> 
>> But if we're in wait_reading_process_output, that means that idle or
>> not, we're checking for process output, so why not check for process
>> status changes as well?

> Because status_notify has a side effect of triggering redisplay.

Thanks.  Now it makes a lot more sense.  Please add a corresponding
comment on the do_display check.


        Stefan

Re: bug#141: Process sentinel not called in batch mode

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> But if we're in wait_reading_process_output, that means that idle or
>>> not, we're checking for process output, so why not check for process
>>> status changes as well?
>
>> Because status_notify has a side effect of triggering redisplay.
>
> Thanks.  Now it makes a lot more sense.  Please add a corresponding
> comment on the do_display check.

What do you think should be the fix for the original problem?

Re: bug#141: Process sentinel not called in batch mode

[Stefan Monnier]

>>>> But if we're in wait_reading_process_output, that means that idle or
>>>> not, we're checking for process output, so why not check for process
>>>> status changes as well?
>> 
>>> Because status_notify has a side effect of triggering redisplay.
>> 
>> Thanks.  Now it makes a lot more sense.  Please add a corresponding
>> comment on the do_display check.

> What do you think should be the fix for the original problem?

The one you proposed seems right,


        Stefan

Re: bug#141: Process sentinel not called in batch mode

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>>> But if we're in wait_reading_process_output, that means that idle or
>>>>> not, we're checking for process output, so why not check for process
>>>>> status changes as well?
>>> 
>>>> Because status_notify has a side effect of triggering redisplay.
>>> 
>>> Thanks.  Now it makes a lot more sense.  Please add a corresponding
>>> comment on the do_display check.
>
>> What do you think should be the fix for the original problem?
>
> The one you proposed seems right,

OK.  I've checked it in.