bug#51177: 29.0.50; stop-process on pipes

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

I would like to request this feature: stop-process, when called with a
sub-process that is connected via pipes, should use delete_read_fd.  It
should basically do the same as for sockets.

Currently, stop-process uses some difficult to understand logic and
eventually sends SIGSTOP to the process.  This might work in theory, but
in practice it can take a long time before Emacs stops receiving output.

The code below illustrates the problem with two examples:
test-stop-process and test-signal-process.  One uses stop-process while
the other uses (signal-process 'SIGSTOP).  The output is something like
this:

  emacs -Q --batch -l stop.el -f ert-run-tests-batch-and-exit
  Running 2 tests (2021-10-13 11:00:45+0200, selector ‘t’)
  use-signals = t; my-counter = 17 buffer-size = 69672
     passed  1/2  test-signal-process (0.200925 sec)
  use-signals = nil; my-counter = 1099 buffer-size = 4501504
     passed  2/2  test-stop-process (0.201133 sec)

With signal-process, the filter function is called 17 times before the
sub-process stops sending output.

With stop-process, it takes 1099 calls and Emacs receives 4 megabytes of
output.  These numbers can be even higher, if the argument to sleep-for
is larger.

If we would use delete_read_fd, then the filter function would be called
exactly once.  At least I think so.  That would, of course, be much more
desirable.

Helmut


(ert-deftest test-stop-process () (run-test nil))

(ert-deftest test-signal-process () (run-test t))

(defun my-start-process ()
  (let ((buffer (generate-new-buffer " some-process")))
    (make-process :command '("cat" "/dev/zero")
                  :name (buffer-name buffer)
                  :buffer buffer
                  :filter #'my-filter
                  :connection-type 'pipe)))

(defvar my-counter 0)

(defun my-filter (proc string)
  (setq my-counter (1+ my-counter))
  (with-current-buffer (process-buffer proc)
    (goto-char (point-max))
    (insert string)
    ;; (message "stopping: %s %s %s" (buffer-size)
    ;;         (process-id proc) (process-status proc))
    (cond ((process-get proc 'use-signals)
           (signal-process proc 'SIGSTOP))
          (t
           (stop-process proc)))))

(defun run-test (use-signals)
  (let ((proc (my-start-process))
        (my-counter 0))
    (process-put proc 'use-signals use-signals)
    (sleep-for 0.2)
    (while (= my-counter 0)
      (accept-process-output p 0.1))
    (message "use-signals = %s; my-counter = %s buffer-size = %s" 
             use-signals my-counter
             (buffer-size (process-buffer proc)))))



In GNU Emacs 29.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2021-10-11 built on caladan
Repository revision: 2810fe6bfca182e4376d818b5510507d5ff7e1b5
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --with-xpm=ifavailable --with-jpeg=ifavailable
 --with-gif=ifavailable --with-tiff=ifavailable'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ LIBSELINUX
LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND
THREADS TOOLKIT_SCROLL_BARS X11 XDBE XIM GTK3 ZLIB

Important settings:
  value of $LANG: C.UTF-8
  locale-coding-system: utf-8-unix

bug#51177: 29.0.50; stop-process on pipes

[Lars Ingebrigtsen]

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

> I would like to request this feature: stop-process, when called with a
> sub-process that is connected via pipes, should use delete_read_fd.  It
> should basically do the same as for sockets.
>
> Currently, stop-process uses some difficult to understand logic and
> eventually sends SIGSTOP to the process.  This might work in theory, but
> in practice it can take a long time before Emacs stops receiving output.

You're supposed to be able to use `continue-process' on the process
after stopping it for a while -- that's not possible if you delete the
fd.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On Wed, Oct 13 2021, Lars Ingebrigtsen wrote:

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> I would like to request this feature: stop-process, when called with a
>> sub-process that is connected via pipes, should use delete_read_fd.  It
>> should basically do the same as for sockets.
>>
>> Currently, stop-process uses some difficult to understand logic and
>> eventually sends SIGSTOP to the process.  This might work in theory, but
>> in practice it can take a long time before Emacs stops receiving output.
>
> You're supposed to be able to use `continue-process' on the process
> after stopping it for a while -- that's not possible if you delete the
> fd.  

Here "deleting" only removes the fd from the event loop; it doesn't
close the fd.  The fd can be added back by `continue-process'.

The patch below implements the basic idea.  Though, not very nicely
because in this case p->command can't be used to indicate that the fd is
temporarily "deleted".  It writes something to the process->plist.  A
proper patch would probably do this in a better way:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: stop.patch --]
[-- Type: text/x-diff, Size: 2313 bytes --]

diff --git a/src/process.c b/src/process.c
index 746cdc0428..5b833187d5 100644
--- a/src/process.c
+++ b/src/process.c
@@ -6921,15 +6921,26 @@ of incoming traffic.  */)
   (Lisp_Object process, Lisp_Object current_group)
 {
   if (PROCESSP (process) && (NETCONN_P (process) || SERIALCONN_P (process)
-			     || PIPECONN_P (process)))
+			     || PIPECONN_P (process)
+			     || (EQ (Fprocess_type (process), Qreal)
+				 && NILP (Fprocess_tty_name (process)))))
     {
       struct Lisp_Process *p;
 
       p = XPROCESS (process);
-      if (NILP (p->command)
+      if ((NILP (p->command)
+	   || (EQ (Fprocess_type (process), Qreal)
+	       && NILP (Fprocess_tty_name (process))))
 	  && p->infd >= 0)
 	delete_read_fd (p->infd);
-      pset_command (p, Qt);
+
+      if (NILP (p->command))
+	pset_command (p, Qt);
+      else if (EQ (Fprocess_type (process), Qreal)
+	       && NILP (Fprocess_tty_name (process)))
+	Fset_process_plist (process,
+			    Fplist_put (Fprocess_plist (process),
+					Qstop, Qt));
       return process;
     }
 #ifndef SIGTSTP
@@ -6948,13 +6959,18 @@ traffic.  */)
   (Lisp_Object process, Lisp_Object current_group)
 {
   if (PROCESSP (process) && (NETCONN_P (process) || SERIALCONN_P (process)
-			     || PIPECONN_P (process)))
+			     || PIPECONN_P (process)
+			     || (EQ (Fprocess_type (process), Qreal)
+				 && NILP (Fprocess_tty_name (process)))))
     {
       struct Lisp_Process *p;
 
       p = XPROCESS (process);
       eassert (p->infd < FD_SETSIZE);
-      if (EQ (p->command, Qt)
+      if ((EQ (p->command, Qt)
+	   || (EQ (Fprocess_type (process), Qreal)
+	       && NILP (Fprocess_tty_name (process))
+	       && EQ (Fplist_get (Fprocess_plist (process), Qstop), Qt)))
 	  && p->infd >= 0
 	  && (!EQ (p->filter, Qt) || EQ (p->status, Qlisten)))
 	{
@@ -6966,7 +6982,14 @@ traffic.  */)
 	  tcflush (p->infd, TCIFLUSH);
 #endif /* not WINDOWSNT */
 	}
-      pset_command (p, Qnil);
+      if (EQ (p->command, Qt))
+	pset_command (p, Qnil);
+      else if (EQ (Fprocess_type (process), Qreal)
+	       && NILP (Fprocess_tty_name (process))
+	       && EQ (Fplist_get (Fprocess_plist (process), Qstop), Qt))
+	Fset_process_plist (process,
+			    Fplist_put (Fprocess_plist (process),
+					Qstop, Qnil));
       return process;
     }
 #ifdef SIGCONT

[-- Attachment #3: Type: text/plain, Size: 8 bytes --]


Helmut

bug#51177: 29.0.50; stop-process on pipes

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Wed, 13 Oct 2021 11:20:01 +0200
> 
> 
> I would like to request this feature: stop-process, when called with a
> sub-process that is connected via pipes, should use delete_read_fd.  It
> should basically do the same as for sockets.

Unlike with sockets, we are talking about a real sub-process on the
other end of the pipe, and it will now get SIGPIPE.  Are we sure this
is OK? perhaps it will interfere with the process's cleanup when it
receives a signal?

If there's any real possibility this could change behavior, I think we
should make such behavior optional.

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Wed, Oct 13 2021, Eli Zaretskii wrote:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> Date: Wed, 13 Oct 2021 11:20:01 +0200
>> 
>> 
>> I would like to request this feature: stop-process, when called with a
>> sub-process that is connected via pipes, should use delete_read_fd.  It
>> should basically do the same as for sockets.
>
> Unlike with sockets, we are talking about a real sub-process on the
> other end of the pipe, and it will now get SIGPIPE.  Are we sure this
> is OK? perhaps it will interfere with the process's cleanup when it
> receives a signal?

A valid concern, yes.

> If there's any real possibility this could change behavior, I think we
> should make such behavior optional.

Maybe we could add a pair of functions that exposes delete_read_fd and
add_process_read_fd more directly.

Helmut

bug#51177: 29.0.50; stop-process on pipes

[jakanakaevangeli]

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

> On Wed, Oct 13 2021, Eli Zaretskii wrote:
>
>>> From: Helmut Eller <eller.helmut@gmail.com>
>>> Date: Wed, 13 Oct 2021 11:20:01 +0200
>>> 
>>> 
>>> I would like to request this feature: stop-process, when called with a
>>> sub-process that is connected via pipes, should use delete_read_fd.  It
>>> should basically do the same as for sockets.
>>
>> Unlike with sockets, we are talking about a real sub-process on the
>> other end of the pipe, and it will now get SIGPIPE.  Are we sure this
>> is OK? perhaps it will interfere with the process's cleanup when it
>> receives a signal?
>
> A valid concern, yes.
>
>> If there's any real possibility this could change behavior, I think we
>> should make such behavior optional.
>
> Maybe we could add a pair of functions that exposes delete_read_fd and
> add_process_read_fd more directly.
>
> Helmut

I haven't read you request and patch in detail, but have you tried
"(set-process-filter proc t)"?  Looking at the doc string of
set-process-filter and reading its code suggests that this may be what
you want.

Best regards.

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Thu, Oct 14 2021, jakanakaevangeli@chiru.no wrote:

> I haven't read you request and patch in detail, but have you tried
> "(set-process-filter proc t)"?  Looking at the doc string of
> set-process-filter and reading its code suggests that this may be what
> you want.

Indeed, this does exactly what I want.  Thank you!

I did not read the docstring but I did read the manual.  This feature is
not described in the manual.  At least not near set-process-filter.

Helmut

bug#51177: 29.0.50; stop-process on pipes

[Lars Ingebrigtsen]

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

> I did not read the docstring but I did read the manual.  This feature is
> not described in the manual.  At least not near set-process-filter.

Yup.  I've now documented the t value in the manual in emacs-28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Thu, Oct 14 2021, Lars Ingebrigtsen wrote:

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> I did not read the docstring but I did read the manual.  This feature is
>> not described in the manual.  At least not near set-process-filter.
>
> Yup.  I've now documented the t value in the manual in emacs-28.

The t value also doesn't seem be handled correctly by make-process:

(ert-deftest test-filter=t ()
  (let ((p (make-process :command '("dd" "if=/dev/zero" "count=0")
			 :name "foo"
			 :filter t)))
    ;;(set-process-filter p t)
    (while (eq (process-status p) 'run)
      (accept-process-output p))))

when executed with

  emacs -Q --batch -l test.el -f ert-run-tests-batch-and-exit

produces:

Running 1 tests (2021-10-16 18:21:53+0200, selector ‘t’)
Test test-filter=t backtrace:
  t(#<process foo> "0+0 records in\n0+0 records out\n")
  accept-process-output(#<process foo>)
  (while (eq (process-status p) 'run) (accept-process-output p))
  (let ((p (make-process :command '("dd" "if=/dev/zero" "count=0") :na
  (lambda nil (let ((p (make-process :command '("dd" "if=/dev/zero" "c
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name test-filter=t :documentation nil :bod
  ert-run-or-rerun-test(#s(ert--stats :selector t :tests [#s(ert-test 
  ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit()
  command-line-1(("-l" "test.el" "-f" "ert-run-tests-batch-and-exit"))
  command-line()
  normal-top-level()
Test test-filter=t condition:
    (void-function t)
   FAILED  1/1  test-filter=t (0.001650 sec)

Ran 1 tests, 0 results as expected, 1 unexpected (2021-10-16 18:21:53+0200, 0.115393 sec)

1 unexpected results:
   FAILED  test-filter=t


Helmut

bug#51177: 29.0.50; stop-process on pipes

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Sat, 16 Oct 2021 18:24:02 +0200
> Cc: 51177@debbugs.gnu.org, jakanakaevangeli@chiru.no
> 
> On Thu, Oct 14 2021, Lars Ingebrigtsen wrote:
> 
> > Helmut Eller <eller.helmut@gmail.com> writes:
> >
> >> I did not read the docstring but I did read the manual.  This feature is
> >> not described in the manual.  At least not near set-process-filter.
> >
> > Yup.  I've now documented the t value in the manual in emacs-28.
> 
> The t value also doesn't seem be handled correctly by make-process:
> 
> (ert-deftest test-filter=t ()
>   (let ((p (make-process :command '("dd" "if=/dev/zero" "count=0")
> 			 :name "foo"
> 			 :filter t)))
>     ;;(set-process-filter p t)
>     (while (eq (process-status p) 'run)
>       (accept-process-output p))))
> 
> when executed with
> 
>   emacs -Q --batch -l test.el -f ert-run-tests-batch-and-exit
> 
> produces:
> 
> Running 1 tests (2021-10-16 18:21:53+0200, selector ‘t’)
> Test test-filter=t backtrace:
>   t(#<process foo> "0+0 records in\n0+0 records out\n")
>   accept-process-output(#<process foo>)

What do we expect to happen when a Lisp program calls
accept-process-output on a process that is stopped?

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Sat, Oct 16 2021, Eli Zaretskii wrote:
>> Running 1 tests (2021-10-16 18:21:53+0200, selector ‘t’)
>> Test test-filter=t backtrace:
>>   t(#<process foo> "0+0 records in\n0+0 records out\n")
>>   accept-process-output(#<process foo>)
>
> What do we expect to happen when a Lisp program calls
> accept-process-output on a process that is stopped?

I would not expect that the symbol t will be called.

I would expect that accept-process-output on a process initialized with

   (make-process ... :filter t)

and

   (set-process-filter (make-process ...) t)

does the same.

I would expect that accept-process-output checks and maybe updates the
process-status.  If the process-status hasn't changed, then the return
value of accept-process-output should be nil.

Helmut

bug#51177: 29.0.50; stop-process on pipes

[Lars Ingebrigtsen]

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

> I would expect that accept-process-output on a process initialized with
>
>    (make-process ... :filter t)
>
> and
>
>    (set-process-filter (make-process ...) t)
>
> does the same.

Yes, I think that makes sense.  And there was some support for it
already, but not in all the code paths.  I've adjusted this in Emacs 29
now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

I have another problem with stopped output.  In this example:

;;; -*- lexical-binding:t -*-

(ert-deftest test-read-after-exit ()
  (let* ((output "")
	 (filter (lambda (p s) (setq output (concat output s))))
	 (sentinel (lambda (p _)
		     (set-process-filter p filter)
		     (while (accept-process-output p 0))))
	 (proc (make-process :command '("printf" "foo")
			     :name "test-proc"
			     :filter t
			     :sentinel sentinel
			     :connection-type 'pipe)))
    (while (process-live-p proc)
      (accept-process-output proc 0.2))
    (set-process-filter proc filter)
    (while (accept-process-output proc 0))
    (should (equal output "foo"))))

the filter function is never called.

We could say that reading the process's output after the process has
terminated is an unreasonable request.  However, I would like to propose
that, in status_notify, the sentinel function should be called before
closing the file descriptors.  That way, the sentinel can read the
buffered output as suggested in the example.

Helmut

bug#51177: 29.0.50; stop-process on pipes

[Lars Ingebrigtsen]

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

> We could say that reading the process's output after the process has
> terminated is an unreasonable request.

I'm not sure I quite understand -- all the output from the process
should be delivered (to the filter function) before Emacs marks the
process as terminated, I think?

> However, I would like to propose
> that, in status_notify, the sentinel function should be called before
> closing the file descriptors.  That way, the sentinel can read the
> buffered output as suggested in the example.

A sentinel usually doesn't read anything...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Fri, Nov 12 2021, Lars Ingebrigtsen wrote:

>> However, I would like to propose
>> that, in status_notify, the sentinel function should be called before
>> closing the file descriptors.  That way, the sentinel can read the
>> buffered output as suggested in the example.
>
> A sentinel usually doesn't read anything...

The idea is that the sentinel does something like this:

  (lambda (p _)
    (set-process-filter p filter)
    (while (accept-process-output p 0)))

First, it changes the filter from t to an actual function.  Then it
calls accept-process-output.  This in turn polls the file descriptors
and calls the filter function if there is buffered output.  If there is
no buffered output to read, then accept-process-output returns nil and
the while loop terminates.

All this happens after the process has terminated.  Granted, not a
particularly intuitive API.

However, the required change would be rather small, I think.  The patch
below shows how this could be done.  It basically moves the part that
closes the file descriptors after the call to exec_sentinel.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sentinel.patch --]
[-- Type: text/x-diff, Size: 1377 bytes --]

diff --git a/src/process.c b/src/process.c
index f923aff1cb..bc236c7e4c 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1341,6 +1341,9 @@ The string argument is normally a multibyte string, except:
                && !EQ (p->command, Qt))
         add_process_read_fd (p->infd);
     }
+  else {
+    fprintf (stderr, "p->infd < 0 in Fset_process_filter\n");
+  }
 
   pset_filter (p, filter);
 
@@ -7536,15 +7539,6 @@ status_notify (struct Lisp_Process *deleting_process,
 	  if (CONSP (p->status))
 	    symbol = XCAR (p->status);
 
-	  if (EQ (symbol, Qsignal) || EQ (symbol, Qexit)
-	      || EQ (symbol, Qclosed))
-	    {
-	      if (delete_exited_processes)
-		remove_process (proc);
-	      else
-		deactivate_process (proc);
-	    }
-
 	  /* The actions above may have further incremented p->tick.
 	     So set p->update_tick again so that an error in the sentinel will
 	     not cause this code to be run again.  */
@@ -7554,6 +7548,16 @@ status_notify (struct Lisp_Process *deleting_process,
 	  if (BUFFERP (p->buffer))
 	    /* In case it uses %s in mode-line-format.  */
 	    bset_update_mode_line (XBUFFER (p->buffer));
+
+	  if (EQ (symbol, Qsignal) || EQ (symbol, Qexit)
+	      || EQ (symbol, Qclosed))
+	    {
+	      if (delete_exited_processes)
+		remove_process (proc);
+	      else
+		deactivate_process (proc);
+	    }
+
 	}
     } /* end for */
 

bug#51177: 29.0.50; stop-process on pipes

[Lars Ingebrigtsen]

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

> All this happens after the process has terminated.  Granted, not a
> particularly intuitive API.

Ah, I see.  No, that's not very intuitive.  😀

As for the patch itself, I'd worry that a subtle change in semantics
here would break stuff (and this is an area that's full of notoriously
subtle things), but perhaps it's OK.  Anybody have an opinion here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51177: 29.0.50; stop-process on pipes

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 12 Nov 2021 07:30:45 +0100
> Cc: 51177@debbugs.gnu.org
> 
> As for the patch itself, I'd worry that a subtle change in semantics
> here would break stuff (and this is an area that's full of notoriously
> subtle things), but perhaps it's OK.  Anybody have an opinion here?

First, the patch included an fprintf that should probably be removed.

And second, I'd prefer to have a variable exposed to Lisp to control
this behavior, so that if someone finds some strange consequences, we
could ask them to flip the variable and see if the problem goes away.

My main worry is what happens if we try reading from a pipe to a
process that died, and so its end of the pipe could be closed.  Was
this patch tested when process-connection-type is nil?

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Fri, Nov 12 2021, Eli Zaretskii wrote:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Fri, 12 Nov 2021 07:30:45 +0100
>> Cc: 51177@debbugs.gnu.org
>> 
>> As for the patch itself, I'd worry that a subtle change in semantics
>> here would break stuff (and this is an area that's full of notoriously
>> subtle things), but perhaps it's OK.  Anybody have an opinion here?

Ideally, there'd be test suite for those subtleties...

> First, the patch included an fprintf that should probably be removed.

Yes, of course.

> And second, I'd prefer to have a variable exposed to Lisp to control
> this behavior, so that if someone finds some strange consequences, we
> could ask them to flip the variable and see if the problem goes away.

Maybe that variable could be the filter itself.  We could delay closing
file descriptors only if the filter==t.  For the other values,
everything could stay as now.

> My main worry is what happens if we try reading from a pipe to a
> process that died, and so its end of the pipe could be closed.  Was
> this patch tested when process-connection-type is nil?

Just tested it.  Works.

I do wonder why the part of the code with the comment "If process is
still active, read any output that remains" is not executed for the
deleting_process.  It seems to me that this creates the possibility (with
very low probability) that we forget to read the last chunk of output in
the usual case where filter ≠ t.

Helmut

bug#51177: 29.0.50; stop-process on pipes

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  51177@debbugs.gnu.org
> Date: Fri, 12 Nov 2021 09:28:49 +0100
> 
> > My main worry is what happens if we try reading from a pipe to a
> > process that died, and so its end of the pipe could be closed.  Was
> > this patch tested when process-connection-type is nil?
> 
> Just tested it.  Works.

On what OS?

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Fri, Nov 12 2021, Eli Zaretskii wrote:

>> > My main worry is what happens if we try reading from a pipe to a
>> > process that died, and so its end of the pipe could be closed.  Was
>> > this patch tested when process-connection-type is nil?
>> 
>> Just tested it.  Works.
>
> On what OS?

On GNU/Linux.

bug#51177: 29.0.50; stop-process on pipes

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: larsi@gnus.org,  51177@debbugs.gnu.org
> Date: Fri, 12 Nov 2021 13:34:24 +0100
> 
> On Fri, Nov 12 2021, Eli Zaretskii wrote:
> 
> >> > My main worry is what happens if we try reading from a pipe to a
> >> > process that died, and so its end of the pipe could be closed.  Was
> >> > this patch tested when process-connection-type is nil?
> >> 
> >> Just tested it.  Works.
> >
> > On what OS?
> 
> On GNU/Linux.

Thanks.  Can you show a recipe for testing this?

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Fri, Nov 12 2021, Eli Zaretskii wrote:

> Thanks.  Can you show a recipe for testing this?

Create a file with this code:

(ert-deftest test-read-after-exit ()
  (let* ((output "")
	 (filter (lambda (p s) (setq output (concat output s))))
	 (sentinel (lambda (p _)
		     (set-process-filter p filter)
		     (while (accept-process-output p 0))))
	 (proc (make-process :command '("printf" "foo")
			     :name "test-proc"
			     :filter t
			     :sentinel sentinel)))
    (while (process-live-p proc)
      (accept-process-output proc 0.2))
    (should (equal output "foo"))))

and execute: emacs -Q --batch -l test.el -f ert-run-tests-batch-and-exit

However, if delete-process is called from the sentinel like so

(ert-deftest test-delete-proces ()
  (let* ((output "")
	 (filter (lambda (p s) (setq output (concat output s))))
	 (sentinel (lambda (p _)
		     (set-process-filter p filter)
		     (while (accept-process-output p 0))
		     (delete-process p)))
	 (proc (make-process :command '("printf" "foo")
			     :name "test-proc"
			     :filter t
			     :sentinel sentinel)))
    (while (process-live-p proc)
      (accept-process-output proc 0.2))
    (should (equal output "foo"))))


then it produces an endless loop.

Helmut

bug#51177: 29.0.50; stop-process on pipes

[Helmut Eller]

On Fri, Nov 12 2021, Helmut Eller wrote:

> I do wonder why the part of the code with the comment "If process is
> still active, read any output that remains" is not executed for the
> deleting_process.  It seems to me that this creates the possibility (with
> very low probability) that we forget to read the last chunk of output in
> the usual case where filter ≠ t.

I see now why.  The Fdelete_process uses (p->infd >= 0) to decide if it
should call status_notify.  I guess this is pretty much a show stopper
as this will result in an endless recursion when delete-process is
called from a sentinel without closing the file descriptors first.

Helmut