From b21f4a3820be8fc579427a9dae20270bfebcbcc9 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Fri, 19 Jul 2024 18:02:16 -0700 Subject: [PATCH 2/2] Use 'unwind-protect' to ensure that Eshell always closes I/O handles * lisp/eshell/esh-cmd.el (eshell-with-handles): New macro... (eshell-commands): ... use it. (eshell-with-copied-handles): Remove STEAL-P and allow multiple body forms (this is an incompatible change, but the macro is currently internal despite the name). (eshell-parse-command, eshell-do-pipelines) (eshell-do-pipelines-synchronously, eshell--invoke-command-directly-p): Remove handle stealing. (eshell-structure-basic-command, eshell-do-command) (eshell-lisp-command): Remove 'eshell-close-handles'. (eshell-protect): Make obsolete. (eshell-rewrite-for-command, eshell-rewrite-while-command) (eshell-rewrite-if-command, (eshell-parse-pipeline): Remove 'eshell-protect'. * lisp/eshell/esh-io.el (eshell-duplicate-handles): Make STEAL-P obsolete. * lisp/eshell/esh-proc.el (eshell-gather-process-output): Call 'eshell-protect-handles' one more time. Remove 'eshell-close-handles'. * lisp/eshell/esh-var.el (eshell-parse-variable-ref): Reimplement $ form using 'eshell-with-handles'. * test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/command-not-found/pipeline): New test. * test/lisp/eshell/em-tramp-tests.el (em-tramp-test/should-replace-command): Adjust check for 'eshell-with-copied-handles'. --- lisp/eshell/esh-cmd.el | 92 ++++++++++++++---------------- lisp/eshell/esh-io.el | 8 +-- lisp/eshell/esh-proc.el | 2 +- lisp/eshell/esh-var.el | 34 ++++++----- test/lisp/eshell/em-tramp-tests.el | 3 +- test/lisp/eshell/esh-cmd-tests.el | 11 ++++ 6 files changed, 76 insertions(+), 74 deletions(-) diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 43d536ac463..0a68859fc0a 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -415,7 +415,6 @@ eshell-parse-command ;; The last command (first in our reversed list) is implicitly ;; terminated by ";". (sep-terms (cons ";" sep-terms)) - (steal-handles t) (commands (nreverse (mapcan @@ -428,11 +427,8 @@ eshell-parse-command (unless eshell-in-pipeline-p (setq cmd `(eshell-do-command ,cmd))) ;; Copy I/O handles so each full statement can manipulate - ;; them if they like. Steal the handles for the last - ;; command (first in our reversed list); we won't use the - ;; originals again anyway. - (setq cmd `(eshell-with-copied-handles ,cmd ,steal-handles) - steal-handles nil) + ;; them if they like. + (setq cmd `(eshell-with-copied-handles ,cmd)) (when (equal sep "&") (setq cmd `(eshell-do-subjob ,cmd))) (list cmd)))) @@ -547,10 +543,8 @@ eshell-rewrite-for-command (let ((,(intern (cadr terms)) (car ,for-items)) (eshell--local-vars (cons ',(intern (cadr terms)) eshell--local-vars))) - (eshell-protect - ,(eshell-invokify-arg body t))) - (setq ,for-items (cdr ,for-items))) - (eshell-close-handles))))) + ,(eshell-invokify-arg body t)) + (setq ,for-items (cdr ,for-items))))))) (defun eshell-structure-basic-command (func names keyword test body &optional else) @@ -577,11 +571,8 @@ eshell-structure-basic-command (string= keyword (cadr names)))) (setq test `(not ,test))) - ;; finally, create the form that represents this structured - ;; command - `(progn - (,func ,test ,body ,else) - (eshell-close-handles))) + ;; Finally, create the form that represents this structured command. + `(,func ,test ,body ,else)) (defun eshell-rewrite-while-command (terms) "Rewrite a `while' command into its equivalent Eshell command form. @@ -593,8 +584,7 @@ eshell-rewrite-while-command (eshell-structure-basic-command 'while '("while" "until") (car terms) (eshell-invokify-arg (cadr terms) nil t) - `(eshell-protect - ,(eshell-invokify-arg (car (last terms)) t))))) + (eshell-invokify-arg (car (last terms)) t)))) (defun eshell-rewrite-if-command (terms) "Rewrite an `if' command into its equivalent Eshell command form. @@ -606,12 +596,9 @@ eshell-rewrite-if-command (eshell-structure-basic-command 'if '("if" "unless") (car terms) (eshell-invokify-arg (cadr terms) nil t) - `(eshell-protect - ,(eshell-invokify-arg (car (last terms (if (= (length terms) 4) 2))) - t)) - (if (= (length terms) 4) - `(eshell-protect - ,(eshell-invokify-arg (car (last terms)) t)))))) + (eshell-invokify-arg (car (last terms (if (= (length terms) 4) 2))) t) + (when (= (length terms) 4) + (eshell-invokify-arg (car (last terms)) t))))) (defun eshell-set-exit-info (status &optional result) "Set the exit status and result for the last command. @@ -662,8 +649,7 @@ eshell-parse-pipeline (cl-assert (car sep-terms)) (setq final (eshell-structure-basic-command 'if (string= (pop sep-terms) "&&") "if" - `(eshell-protect ,(pop results)) - `(eshell-protect ,final)))) + (pop results) final))) final)) (defun eshell-parse-subcommand-argument () @@ -762,6 +748,28 @@ eshell-separate-commands ;; that `eshell-do-eval' will evaluated, such as command rewriting ;; hooks (see `eshell-rewrite-command-hook' and friends). +(defmacro eshell-with-handles (handle-args &rest body) + "Create a new set of I/O handles and evaluate BODY. +HANDLE-ARGS is a list of arguments to pass to `eshell-create-handles'. +After evaluating BODY, automatically release the handles, allowing them +to close." + (declare (indent 1)) + `(let ((eshell-current-handles (eshell-create-handles ,@handle-args))) + (unwind-protect + ,(if (length= body 1) (car body) `(progn ,@body)) + (eshell-close-handles)))) + +(defmacro eshell-with-copied-handles (&rest body) + "Copy the current I/O handles and evaluate BODY. +After evaluating BODY, automatically release the handles, allowing them +to close." + (declare (indent 0)) + `(let ((eshell-current-handles + (eshell-duplicate-handles eshell-current-handles))) + (unwind-protect + ,(if (length= body 1) (car body) `(progn ,@body)) + (eshell-close-handles)))) + (defmacro eshell-do-subjob (object) "Evaluate a command OBJECT as a subjob. We indicate that the process was run in the background by @@ -774,10 +782,9 @@ eshell-do-subjob (defmacro eshell-commands (object &optional silent) "Place a valid set of handles, and context, around command OBJECT." - `(let ((eshell-current-handles - (eshell-create-handles ,(not silent) 'append)) - eshell-current-subjob-p) - ,object)) + `(let (eshell-current-subjob-p) + (eshell-with-handles (,(not silent) 'append) + ,object))) (defvar eshell-this-command-hook nil) @@ -796,8 +803,7 @@ eshell-do-command (mapc #'funcall eshell-this-command-hook))) (error (eshell-errorn (error-message-string err)) - (eshell-set-exit-info 1) - (eshell-close-handles)))) + (eshell-set-exit-info 1)))) (define-obsolete-function-alias 'eshell-trap-errors #'eshell-do-command "31.1") @@ -806,19 +812,12 @@ 'eshell-deferrable If the wrapped form returns a process (or list thereof), Eshell will wait for completion in the background for the process(es) to complete.") -(defmacro eshell-with-copied-handles (object &optional steal-p) - "Duplicate current I/O handles, so OBJECT works with its own copy. -If STEAL-P is non-nil, these new handles will be stolen from the -current ones (see `eshell-duplicate-handles')." - `(let ((eshell-current-handles - (eshell-duplicate-handles eshell-current-handles ,steal-p))) - ,object)) - (define-obsolete-function-alias 'eshell-copy-handles #'eshell-with-copied-handles "30.1") (defmacro eshell-protect (object) "Protect I/O handles, so they aren't get closed after eval'ing OBJECT." + (declare (obsolete nil "31.1")) `(progn (eshell-protect-handles eshell-current-handles) ,object)) @@ -845,9 +844,7 @@ eshell-do-pipelines `(eshell-set-output-handle ,eshell-output-handle 'append (car next-procs))) (let ((proc ,(car pipeline))) - (cons proc next-procs))) - ;; Steal handles if this is the last item in the pipeline. - ,(null (cdr pipeline))))) + (cons proc next-procs)))))) (defmacro eshell-do-pipelines-synchronously (pipeline) "Execute the commands in PIPELINE in sequence synchronously. @@ -869,9 +866,7 @@ eshell-do-pipelines-synchronously ;; meaning for synchronous processes: it's non-nil ;; only when piping *to* a process. (eshell-in-pipeline-p ,(and (cdr pipeline) t))) - ,(car pipeline))) - ;; Steal handles if this is the last item in the pipeline. - ,(null (cdr pipeline))) + ,(car pipeline)))) ,(when (cdr pipeline) `(eshell-do-pipelines-synchronously (quote ,(cdr pipeline))))))) @@ -946,7 +941,7 @@ eshell--invoke-command-directly-p * The command is of the form (eshell-with-copied-handles - (eshell-do-command (eshell-named-command NAME [ARGS])) _). + (eshell-do-command (eshell-named-command NAME [ARGS]))). * NAME is a string referring to an alias function and isn't a complex command (see `eshell-complex-commands'). @@ -954,8 +949,7 @@ eshell--invoke-command-directly-p * Any subcommands in ARGS can also be invoked directly." (pcase command (`(eshell-with-copied-handles - (eshell-do-command (eshell-named-command ,name . ,args)) - ,_) + (eshell-do-command (eshell-named-command ,name . ,args))) (and name (stringp name) (not (member name eshell-complex-commands)) (catch 'simple @@ -1557,7 +1551,7 @@ eshell-lisp-command (not result)) 2) result)) - (eshell-close-handles)))) + nil))) (define-obsolete-function-alias 'eshell-lisp-command* #'eshell-lisp-command "31.1") diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el index 5fccc4fe82f..a6df75e86e9 100644 --- a/lisp/eshell/esh-io.el +++ b/lisp/eshell/esh-io.el @@ -353,14 +353,14 @@ eshell-create-handles (defun eshell-duplicate-handles (handles &optional steal-p) "Create a duplicate of the file handles in HANDLES. This uses the targets of each handle in HANDLES, incrementing its -reference count by one (unless STEAL-P is non-nil). These -targets are shared between the original set of handles and the -new one, so the targets are only closed when the reference count -drops to 0 (see `eshell-close-handles'). +reference count by one. These targets are shared between the original +set of handles and the new one, so the targets are only closed when the +reference count drops to 0 (see `eshell-close-handles'). This function also sets the DEFAULT field for each handle to t (see `eshell-create-handles'). Unlike the targets, this value is not shared with the original handles." + (declare (advertised-calling-convention (handles) "31.1")) (let ((dup-handles (make-vector eshell-number-of-handles nil))) (dotimes (idx eshell-number-of-handles) (when-let ((handle (aref handles idx))) diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el index dc7b497666b..b579a93e14c 100644 --- a/lisp/eshell/esh-proc.el +++ b/lisp/eshell/esh-proc.el @@ -371,6 +371,7 @@ eshell-gather-process-output #'eshell-insertion-filter) :sentinel #'eshell-sentinel)) (eshell-record-process-properties stderr-proc eshell-error-handle)) + (eshell-protect-handles eshell-current-handles) (setq proc (let ((command (file-local-name (expand-file-name command))) (conn-type (pcase (bound-and-true-p eshell-in-pipeline-p) @@ -468,7 +469,6 @@ eshell-gather-process-output (eshell-set-exit-info (if (numberp exit-status) exit-status -1) (and (numberp exit-status) (= exit-status 0))) - (eshell-close-handles) (run-hook-with-args 'eshell-kill-hook command exit-status) (or (bound-and-true-p eshell-in-pipeline-p) (setq eshell-last-sync-output-start nil)) diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el index f0270aca92c..0e6c01d2774 100644 --- a/lisp/eshell/esh-var.el +++ b/lisp/eshell/esh-var.el @@ -553,24 +553,22 @@ eshell-parse-variable-ref (subcmd (or (eshell-unescape-inner-double-quote end) (cons (point) end)))) (prog1 - `(let ((eshell-current-handles - (eshell-create-handles ,temp 'overwrite))) - (progn - (eshell-as-subcommand - ,(let ((eshell-current-quoted nil)) - (eshell-parse-command subcmd))) - (ignore - (nconc eshell-this-command-hook - ;; Quote this lambda; it will be evaluated by - ;; `eshell-do-eval', which requires very - ;; particular forms in order to work - ;; properly. See bug#54190. - (list (function - (lambda () - (delete-file ,temp) - (when-let ((buffer (get-file-buffer ,temp))) - (kill-buffer buffer))))))) - (eshell-apply-indices ,temp indices ,eshell-current-quoted))) + `(eshell-with-handles (,temp 'overwrite) + (eshell-as-subcommand + ,(let ((eshell-current-quoted nil)) + (eshell-parse-command subcmd))) + (ignore + (nconc eshell-this-command-hook + ;; Quote this lambda; it will be evaluated by + ;; `eshell-do-eval', which requires very + ;; particular forms in order to work + ;; properly. See bug#54190. + (list (function + (lambda () + (delete-file ,temp) + (when-let ((buffer (get-file-buffer ,temp))) + (kill-buffer buffer))))))) + (eshell-apply-indices ,temp indices ,eshell-current-quoted)) (goto-char (1+ end)))))) ((eq (char-after) ?\() (condition-case nil diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el index 49dd5a78c3d..b85dfc61580 100644 --- a/test/lisp/eshell/em-tramp-tests.el +++ b/test/lisp/eshell/em-tramp-tests.el @@ -29,8 +29,7 @@ em-tramp-test/should-replace-command `(should (equal (catch 'eshell-replace-command ,form) (list 'eshell-with-copied-handles - (list 'eshell-do-command ,replacement) - t)))) + (list 'eshell-do-command ,replacement))))) (ert-deftest em-tramp-test/su-default () "Test Eshell `su' command with no arguments." diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el index 18ea1f9a9d6..cac349a2616 100644 --- a/test/lisp/eshell/esh-cmd-tests.el +++ b/test/lisp/eshell/esh-cmd-tests.el @@ -558,6 +558,17 @@ esh-cmd-test/throw ;; Make sure we can call another command after throwing. (eshell-match-command-output "echo again" "\\`again\n"))) +(ert-deftest esh-cmd-test/command-not-found/pipeline () + "Ensure that processes are stopped if a command in a pipeline is not found." + (skip-when (or (not (executable-find "cat")) + (executable-find "nonexist"))) + (with-temp-eshell + (let ((starting-process-list (process-list))) + (eshell-match-command-output "nonexist | *cat" + "\\`nonexist: command not found\n") + (eshell-wait-for-subprocess t) + (should (equal (process-list) starting-process-list))))) + ;; `which' command -- 2.25.1