Re: master 7f631a3e2ac: Allow using multiple buffers in 'eshell-command'

Re: master 7f631a3e2ac: Allow using multiple buffers in 'eshell-command'

[Stefan Kangas]

Jim Porter <jporterbugs@gmail.com> writes:

> branch: master
> commit 7f631a3e2aca97e95b8659c902c25ab21f084e08
> Author: Thierry Volpiatto <thievol@posteo.net>
> Commit: Jim Porter <jporterbugs@gmail.com>
>
>     Allow using multiple buffers in 'eshell-command'
>
>     Provide the same functionality as 'async-shell-command-buffer' but for
>     'eshell-command'.
>
>     Co-Authored-By: Jim Porter <jporterbugs@gmail.com>
>
>     * lisp/eshell/eshell.el (eshell-command-async-buffer): New option...
>     (eshell-command): ... use it.
>
>     * lisp/eshell/esh-proc.el (eshell-sentinel): Check for buffer liveness
>     in 'finish-io'.
>
>     * test/lisp/eshell/eshell-tests.el
>     (eshell-test/eshell-command/output-buffer/async-kill): New test.

This new test is failing here:

Test eshell-test/eshell-command/output-buffer/async-kill backtrace:
  rename-buffer("*Eshell Async Command Output*" nil)
  eshell-command("*echo bye &")
  #f(compiled-function () #<bytecode -0x9447a45fbdbd42>)()
  #f(compiled-function () #<bytecode 0x4e527f523db3b2e>)()
  handler-bind-1(#f(compiled-function () #<bytecode 0x4e527f523db3b2e>
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name eshell-test/eshell-command/output-buf
  ert-run-or-rerun-test(#s(ert--stats :selector (not ...) :tests [...
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "--eval" "(setq treesit-extra-l
  command-line()
  normal-top-level()
Test eshell-test/eshell-command/output-buffer/async-kill condition:
    (error "Buffer name ‘*Eshell Async Command Output*’ is in use")
   FAILED   5/16  eshell-test/eshell-command/output-buffer/async-kill
(0.005699 sec) at lisp/eshell/eshell-tests.el:120

Re: master 7f631a3e2ac: Allow using multiple buffers in 'eshell-command'

[Jim Porter]

On 7/6/2024 10:15 AM, Stefan Kangas wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> branch: master
>> commit 7f631a3e2aca97e95b8659c902c25ab21f084e08
>> Author: Thierry Volpiatto <thievol@posteo.net>
>> Commit: Jim Porter <jporterbugs@gmail.com>
>>
>>      Allow using multiple buffers in 'eshell-command'
[snip]
> This new test is failing here:

Hm, I can't reproduce that, but I'm guessing it's a race condition with 
the process sentinel. Does this make the test pass for you?

----------------------------------------

diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index 568f6745067..6c79f753d6f 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -357,8 +357,8 @@ eshell-command
                (with-current-buffer bufname
                  ;; Stop all the processes in the old buffer (there may
                  ;; be several).
-                (eshell-process-interact #'interrupt-process t))
-              (accept-process-output)
+                (eshell-process-interact #'interrupt-process t)
+                (eshell-wait-for-process (mapcar #'car 
eshell-process-list)))
                (kill-buffer bufname))
               ((eq eshell-command-async-buffer 'confirm-new-buffer)
                (shell-command--same-buffer-confirm "Use a new buffer")

Re: master 7f631a3e2ac: Allow using multiple buffers in 'eshell-command'

[Jim Porter]

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

On 7/6/2024 12:47 PM, Jim Porter wrote:
> On 7/6/2024 10:15 AM, Stefan Kangas wrote:
>> Jim Porter <jporterbugs@gmail.com> writes:
>>
>>> branch: master
>>> commit 7f631a3e2aca97e95b8659c902c25ab21f084e08
>>> Author: Thierry Volpiatto <thievol@posteo.net>
>>> Commit: Jim Porter <jporterbugs@gmail.com>
>>>
>>>      Allow using multiple buffers in 'eshell-command'
> [snip]
>> This new test is failing here:
> 
> Hm, I can't reproduce that, but I'm guessing it's a race condition with 
> the process sentinel. Does this make the test pass for you?

Scratch that patch. I looked into this code more, and the following 
should (hopefully) fix the test, as well as improving Eshell's behavior 
when waiting for processes more generally.

[-- Attachment #2: 0001-Improve-Eshell-s-behavior-when-waiting-for-processes.patch --]
[-- Type: text/plain, Size: 8006 bytes --]

From a7ace1e286333dde0b20b1a6cdff80c5751bcca4 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 6 Jul 2024 14:09:08 -0700
Subject: [PATCH] Improve Eshell's behavior when waiting for processes

This has a few benefits.  First, it fixes a race condition when killing
old processes in 'eshell-command'.  Second, the "wait" built-in command
is now more useful.  Finally, killing processes when exiting Eshell (via
'eshell-round-robin-kill') should be much faster.

* lisp/eshell/esh-proc.el (esh-opt): Require.
(eshell-wait-for-process): Make obsolete in favor of...
(eshell-wait-for-processes): ... this.  Accept a timeout and support
PIDs.  Update callers.
(eshell/wait): New implementation accepting -t/--timeout.
(eshell-round-robin-kill): Use 'eshell-wait-for-processes'.

* lisp/eshell/eshell.el (eshell-command): Use 'eshell-round-robin-kill'.

* doc/misc/eshell.texi (List of Built-ins): Document the new "wait"
behavior.

* etc/NEWS: Announce this change.
---
 doc/misc/eshell.texi    |  7 +++--
 etc/NEWS                |  6 ++++
 lisp/eshell/esh-cmd.el  |  2 +-
 lisp/eshell/esh-proc.el | 65 +++++++++++++++++++++++++++++------------
 lisp/eshell/eshell.el   |  7 ++---
 5 files changed, 62 insertions(+), 25 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 69f94fab469..8547131194e 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -1201,8 +1201,11 @@ List of Built-ins
 
 @cmindex wait
 @cindex processes, waiting for
-@item wait [@var{process}]@dots{}
-Wait until each specified @var{process} has exited.
+@item wait [-t @var{timeout}] [@var{process}]@dots{}
+Wait until each specified @var{process} has exited.  Processes can
+either be process objects (@pxref{Processes, , , elisp, GNU Emacs Lisp
+Reference Manual}) or integer PIDs.  If you pass @code{-t} or
+@code{--timeout}, wait at most that many seconds before exiting.
 
 @cmindex which
 @item which @var{command}@dots{}
diff --git a/etc/NEWS b/etc/NEWS
index ba58fa7b319..720c194e094 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -54,6 +54,12 @@ this will prompt for confirmation before creating a new buffer when
 necessary.  To restore the previous behavior, set this option to
 'confirm-kill-process'.
 
++++
+*** Eshell's built-in "wait" command now accepts a timeout.
+By passing "-t" or "--timeout", you can specify a maximum time to wait
+for the processes to exit.  Additionally, you can now wait for external
+processes by passing their PIDs.
+
 ** SHR
 
 +++
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index aaae19df5d6..f086aa20181 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1300,7 +1300,7 @@ eshell-do-eval
               (if-let (((memq (car form) eshell-deferrable-commands))
                        (procs (eshell-make-process-list result)))
                   (if synchronous-p
-		      (apply #'eshell/wait procs)
+		      (funcall #'eshell-wait-for-processes procs)
 		    (eshell-manipulate form "inserting ignore form"
 		      (setcar form 'ignore)
 		      (setcdr form nil))
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index a5e9de79907..94be0ee1d3c 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -25,6 +25,7 @@
 
 (require 'esh-arg)
 (require 'esh-io)
+(require 'esh-opt)
 (require 'esh-util)
 
 (require 'pcomplete)
@@ -184,16 +185,46 @@ eshell-process-active-p
       ;; cleared out the handles (see `eshell-sentinel').
       (process-get process :eshell-handles)))
 
-(defun eshell-wait-for-process (&rest procs)
-  "Wait until PROCS have successfully completed."
-  (dolist (proc procs)
-    (when (eshell-processp proc)
-      (while (eshell-process-active-p proc)
-        (when (input-pending-p)
-          (discard-input))
-        (sit-for eshell-process-wait-time)))))
+(defun eshell-wait-for-processes (&optional procs timeout)
+  "Wait until PROCS have completed execution.
+If TIMEOUT is non-nil, wait at most that many seconds.  Return non-nil
+if all the processes finished executing before the timeout expired."
+  (let ((expiration (when timeout (time-add (current-time) timeout))))
+    (dolist (proc procs)
+      (unless (or (processp proc) (natnump proc))
+        (error "Invalid process %S" proc)))
+    (catch 'timeout
+      (dolist (proc procs)
+        (while (if (processp proc)
+                   (eshell-process-active-p proc)
+                 (process-attributes proc))
+          (when (input-pending-p)
+            (discard-input))
+          (when (and expiration
+                     (not (time-less-p (current-time) expiration)))
+            (throw 'timeout nil))
+          (sit-for eshell-process-wait-time)))
+      t)))
 
-(defalias 'eshell/wait #'eshell-wait-for-process)
+(defun eshell-wait-for-process (&rest procs)
+  "Wait until PROCS have completed execution."
+  (declare (obsolete 'eshell-wait-for-processes "31.1"))
+  (eshell-wait-for-processes procs))
+
+(defun eshell/wait (&rest args)
+  "Wait until processes have completed execution."
+  (eshell-eval-using-options
+   "wait" args
+   '((?h "help" nil nil "show this usage screen")
+     (?t "timeout" t timeout "timeout in seconds")
+     :preserve-args
+     :show-usage
+     :usage "[OPTION] PROCESS...
+Wait until PROCESS(es) have completed execution.")
+   (when (stringp timeout)
+     (setq timeout (string-to-number timeout)))
+   (unless (eshell-wait-for-processes args timeout)
+     (error "wait: timed out after %s seconds" timeout))))
 
 (defun eshell/jobs ()
   "List processes, if there are any."
@@ -621,16 +652,14 @@ eshell-kill-processes-on-exit
 (defun eshell-round-robin-kill (&optional query)
   "Kill current process by trying various signals in sequence.
 See the variable `eshell-kill-processes-on-exit'."
-  (let ((sigs eshell-kill-process-signals))
-    (while sigs
+  (catch 'done
+    (dolist (sig eshell-kill-process-signals)
       (eshell-process-interact
-       (lambda (proc)
-         (signal-process (process-id proc) (car sigs))) t query)
-      (setq query nil)
-      (if (not eshell-process-list)
-	  (setq sigs nil)
-	(sleep-for eshell-kill-process-wait-time)
-	(setq sigs (cdr sigs))))))
+       (lambda (proc) (signal-process proc sig)) t query)
+      (when (eshell-wait-for-processes (mapcar #'car eshell-process-list)
+                                       eshell-kill-process-wait-time)
+        (throw 'done nil))
+      (setq query nil))))
 
 (defun eshell-query-kill-processes ()
   "Kill processes belonging to the current Eshell buffer, possibly with query."
diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index 568f6745067..b7be3dd1643 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -176,7 +176,7 @@
   (require 'cl-lib))
 (require 'esh-util)
 (require 'esh-module)                   ;For eshell-using-module
-(require 'esh-proc)                     ;For eshell-wait-for-process
+(require 'esh-proc)                     ;For eshell-wait-for-processes
 (require 'esh-io)                       ;For eshell-last-command-status
 (require 'esh-cmd)
 
@@ -357,8 +357,7 @@ eshell-command
               (with-current-buffer bufname
                 ;; Stop all the processes in the old buffer (there may
                 ;; be several).
-                (eshell-process-interact #'interrupt-process t))
-              (accept-process-output)
+                (eshell-round-robin-kill))
               (kill-buffer bufname))
              ((eq eshell-command-async-buffer 'confirm-new-buffer)
               (shell-command--same-buffer-confirm "Use a new buffer")
@@ -377,7 +376,7 @@ eshell-command
 	;; make the output as attractive as possible, with no
 	;; extraneous newlines
         (unless async
-	  (apply #'eshell-wait-for-process (cadr eshell-foreground-command))
+	  (funcall #'eshell-wait-for-processes (cadr eshell-foreground-command))
 	  (cl-assert (not eshell-foreground-command))
 	  (goto-char (point-max))
 	  (while (and (bolp) (not (bobp)))
-- 
2.25.1

Re: master 7f631a3e2ac: Allow using multiple buffers in 'eshell-command'

[Jim Porter]

On 7/6/2024 2:17 PM, Jim Porter wrote:
> Scratch that patch. I looked into this code more, and the following 
> should (hopefully) fix the test, as well as improving Eshell's behavior 
> when waiting for processes more generally.

Pushed a lightly-modified version of this patch to the master branch as 
8e46f44ea0e. Just let me know if things are still busted.