bug#64975: 30.0.50; accept-process-output and async connect

bug#64975: 30.0.50; accept-process-output and async connect

[Helmut Eller]

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

This example fails:


[-- Attachment #2: async-connect.el --]
[-- Type: text/plain, Size: 467 bytes --]


(ert-deftest async-connect ()
  (let* ((host 'local)
	 (family 'ipv4)
	 (port 57869)
	 (server (make-network-process
		  :name "server" :server t :noquery t :reuseaddr t
		  :host host :service port :family family))
	 (proc (make-network-process
		:name "async-connect" :nowait t
		:host host :service port :family family)))
    (should (eq (process-status proc) 'connect))
    (should (accept-process-output proc 2))
    (should (eq (process-status proc) 'open))))

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


when executed with
  emacs --batch -Q -l async-connect.el -f ert-run-tests-batch-and-exit

It seems that accept-process-output correctly updates the process-status
but it forgets to break out of the loop.

With the following change, the test passes:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: async-wait.patch --]
[-- Type: text/x-diff, Size: 466 bytes --]

diff --git a/src/process.c b/src/process.c
index 2d6e08f16b5..159d39aeabc 100644
--- a/src/process.c
+++ b/src/process.c
@@ -6024,6 +6024,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 #endif
 		    {
 		      pset_status (p, Qrun);
+		      got_some_output = 1;
 		      /* Execute the sentinel here.  If we had relied on
 			 status_notify to do it later, it will read input
 			 from the process before calling the sentinel.  */

[-- Attachment #5: Type: text/plain, Size: 794 bytes --]




In GNU Emacs 30.0.50 (build 73, x86_64-pc-linux-gnu, GTK+ Version
 3.24.37, cairo version 1.16.0) of 2023-07-31 built on caladan
Repository revision: 1f3995f65a065a28e108653128b31a2fb7eeb01c
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux 12 (bookworm)

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 JPEG LIBSELINUX
LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND
SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 GTK3
ZLIB

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

bug#64975: 30.0.50; accept-process-output and async connect

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Mon, 31 Jul 2023 15:31:00 +0200
> 
> (ert-deftest async-connect ()
>   (let* ((host 'local)
> 	 (family 'ipv4)
> 	 (port 57869)
> 	 (server (make-network-process
> 		  :name "server" :server t :noquery t :reuseaddr t
> 		  :host host :service port :family family))
> 	 (proc (make-network-process
> 		:name "async-connect" :nowait t
> 		:host host :service port :family family)))
>     (should (eq (process-status proc) 'connect))
>     (should (accept-process-output proc 2))
>     (should (eq (process-status proc) 'open))))
> 
> when executed with
>   emacs --batch -Q -l async-connect.el -f ert-run-tests-batch-and-exit
> 
> It seems that accept-process-output correctly updates the process-status
> but it forgets to break out of the loop.
> 
> With the following change, the test passes:

Robert, any comments?

bug#64975: 30.0.50; accept-process-output and async connect

[Robert Pluim]

>>>>> On Sat, 05 Aug 2023 12:26:13 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Helmut Eller <eller.helmut@gmail.com>
    >> Date: Mon, 31 Jul 2023 15:31:00 +0200
    >> 
    >> (ert-deftest async-connect ()
    >> (let* ((host 'local)
    >> (family 'ipv4)
    >> (port 57869)
    >> (server (make-network-process
    >> :name "server" :server t :noquery t :reuseaddr t
    >> :host host :service port :family family))
    >> (proc (make-network-process
    >> :name "async-connect" :nowait t
    >> :host host :service port :family family)))
    >> (should (eq (process-status proc) 'connect))
    >> (should (accept-process-output proc 2))
    >> (should (eq (process-status proc) 'open))))
    >> 
    >> when executed with
    >> emacs --batch -Q -l async-connect.el -f ert-run-tests-batch-and-exit
    >> 
    >> It seems that accept-process-output correctly updates the process-status
    >> but it forgets to break out of the loop.
    >> 
    >> With the following change, the test passes:

    Eli> Robert, any comments?

I go away for a week, and this is what you give me as a coming-back
present? :-)

I think itʼs correct, as I have a change locally setting
got_some_output for a different test case, but Iʼm going to be a pain,
and ask Helmut to explain why, and see if I agree with his explanation
(thatʼs a very hairy loop)

Robert
-- 

bug#64975: 30.0.50; accept-process-output and async connect

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  64975@debbugs.gnu.org
> Date: Tue, 08 Aug 2023 11:10:18 +0200
> 
> >>>>> On Sat, 05 Aug 2023 12:26:13 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
>     Eli> Robert, any comments?
> 
> I go away for a week, and this is what you give me as a coming-back
> present? :-)

;-)

> I think itʼs correct, as I have a change locally setting
> got_some_output for a different test case, but Iʼm going to be a pain,
> and ask Helmut to explain why, and see if I agree with his explanation
> (thatʼs a very hairy loop)

I would like to know that as well, and I thought you already knew.

bug#64975: 30.0.50; accept-process-output and async connect

[Robert Pluim]

>>>>> On Tue, 08 Aug 2023 15:15:37 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> I think itʼs correct, as I have a change locally setting
    >> got_some_output for a different test case, but Iʼm going to be a pain,
    >> and ask Helmut to explain why, and see if I agree with his explanation
    >> (thatʼs a very hairy loop)

    Eli> I would like to know that as well, and I thought you already knew.

Itʼs code I last looked at in detail 2 months ago, so the workings are
not completely in memory, and Iʼve got things to do, so Iʼm taking the
lazy option.

Robert
-- 

bug#64975: 30.0.50; accept-process-output and async connect

[Helmut Eller]

On Tue, Aug 08 2023, Robert Pluim wrote:
> I think itʼs correct, as I have a change locally setting
> got_some_output for a different test case, but Iʼm going to be a pain,
> and ask Helmut to explain why, and see if I agree with his explanation
> (thatʼs a very hairy loop)

Setting got_some_output=1, was the first thing that came to mind that
made the test case pass :-).  A reasonable strategy, if we have a
comprehensive test suite.  If the test suite is lacking, then writing
more tests is a good investment too.  Ahem.

Setting got_some_output=1 will terminate the while(1) loop, but only on
the next iteration (around line process.c:5753) and only after another
useless call to xg_select.  So maybe a change like below might be
better.

The variable got_some_output is also the return value of
wait_reading_process_output.  So I thought that 1 is a reasonable value
to indicate "some event happened".  0 and negative values are converted
to nil in accept-process-output, so there isn't an obvious way to
indicate "not a timeout, 0 bytes read, but some other event".  Maybe
MAX_INT could be used.

If you're asking why accept-process-output should return at all, then
the answer is that the socket is now writable and the caller probably
want's to know that.

Helmut

diff --git a/src/process.c b/src/process.c
index 2d6e08f16b5..f5fec84b53c 100644
--- a/src/process.c
+++ b/src/process.c
@@ -6028,6 +6028,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 			 status_notify to do it later, it will read input
 			 from the process before calling the sentinel.  */
 		      exec_sentinel (proc, build_string ("open\n"));
+		      got_some_output = max (got_some_output, 1);
+		      if (p == wait_proc)
+			goto done_waiting;
 		    }
 
 		  if (0 <= p->infd && !EQ (p->filter, Qt)
@@ -6038,6 +6041,8 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	}			/* End for each file descriptor.  */
     }				/* End while exit conditions not met.  */
 
+
+ done_waiting:
   unbind_to (count, Qnil);
 
   /* If calling from keyboard input, do not quit

bug#64975: 30.0.50; accept-process-output and async connect

[Robert Pluim]

>>>>> On Tue, 08 Aug 2023 16:36:00 +0200, Helmut Eller <eller.helmut@gmail.com> said:

    Helmut> On Tue, Aug 08 2023, Robert Pluim wrote:
    >> I think itʼs correct, as I have a change locally setting
    >> got_some_output for a different test case, but Iʼm going to be a pain,
    >> and ask Helmut to explain why, and see if I agree with his explanation
    >> (thatʼs a very hairy loop)

    Helmut> Setting got_some_output=1, was the first thing that came to mind that
    Helmut> made the test case pass :-).  A reasonable strategy, if we have a
    Helmut> comprehensive test suite.  If the test suite is lacking, then writing
    Helmut> more tests is a good investment too.  Ahem.

Submitting new tests is always good :-)

    Helmut> Setting got_some_output=1 will terminate the while(1) loop, but only on
    Helmut> the next iteration (around line process.c:5753) and only after another
    Helmut> useless call to xg_select.  So maybe a change like below might be
    Helmut> better.

It also means we finish looping through all the channels, unlike your
patch below. I think thatʼs a smaller and thus better change, and
aligns more with the docstring:

    Allow any pending output from subprocesses to be read by Emacs.
    It is given to their filter functions.

So if the rest of the test cases pass, I think we should apply your
original patch.

    Helmut> The variable got_some_output is also the return value of
    Helmut> wait_reading_process_output.  So I thought that 1 is a reasonable value
    Helmut> to indicate "some event happened".  0 and negative values are converted
    Helmut> to nil in accept-process-output, so there isn't an obvious way to
    Helmut> indicate "not a timeout, 0 bytes read, but some other event".  Maybe
    Helmut> MAX_INT could be used.

1 is ok as a value.

    Helmut> If you're asking why accept-process-output should return at all, then
    Helmut> the answer is that the socket is now writable and the caller probably
    Helmut> want's to know that.

I think thatʼs ok, since any actual input received from the process
will get passed to the filter function anyway.

Eli, the docstring also says

    Optional argument PROCESS means to return only after output is
    received from PROCESS or PROCESS closes the connection.

Do we need to add something like "or the underlying network connection
becomes available"? (I wonder if thatʼs too strong a guarantee).

Thanks

Robert
-- 

bug#64975: 30.0.50; accept-process-output and async connect

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  64975@debbugs.gnu.org
> Date: Tue, 08 Aug 2023 17:16:52 +0200
> 
> Eli, the docstring also says
> 
>     Optional argument PROCESS means to return only after output is
>     received from PROCESS or PROCESS closes the connection.
> 
> Do we need to add something like "or the underlying network connection
> becomes available"? (I wonder if thatʼs too strong a guarantee).

I'd leave this alone for now.  Connection event was always treated as
"output".