From deeeba9613732c1624fe841928574530a4e453ba Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Sat, 24 Dec 2022 14:31:50 -0800 Subject: [PATCH] Fix reference-counting of Eshell I/O handles This ensures that output targets in Eshell are only closed when Eshell is actually done with them. In particular, this means that "{ echo foo; echo bar } | rev" prints "raboof" as expected (bug#59545). * lisp/eshell/esh-io.el (eshell-create-handles): Structure the handles differently so the targets and their ref-count can be shared. (eshell-duplicate-handles): Reimplement this to share targets between the original and new handle sets. Add STEAL-P argument. (eshell-protect-handles, eshell-copy-output-handle) (eshell-interactive-output-p, eshell-output-object): Account for changes to the handle structure. (eshell-close-handle): New function... (eshell-close-handles, eshell-set-output-handle): ... use it. (eshell-get-targets): Remove. This only existed to make the previous implementation of 'eshell-duplicate-handles' work. * lisp/eshell/esh-cmd.el (eshell-with-copied-handles): New argument STEAL-P. (eshell-do-pipelines): Use STEAL-P for the last item in the pipeline. (eshell-parse-command): Don't copy handles for the last command in the list; explain why we can't use STEAL-P here. (eshell-eval-command): When queuing input, set 'eshell-command-body' and 'eshell-test-body' for the 'if' conditional (see 'eshell-do-eval'). * test/lisp/eshell/esh-io-tests.el (esh-io-test/redirect-pipe): Split into... (esh-io-test/pipeline/default, esh-io-test/pipeline/all): ... these. (esh-io-test/pipeline/subcommands): New test. * test/lisp/eshell/eshell-test-helpers.el (eshell-test--max-subprocess-time): Rename to... (eshell-test--max-wait-time): ... this. (eshell-wait-for): New function... (eshell-wait-for-subprocess): ... use it. * test/lisp/eshell/eshell-tests.el (eshell-test/queue-input): Fix this test. Previously, it didn't correctly verify that the original command completed. * test/lisp/eshell/em-tramp-tests.el (em-tramp-test/should-replace-command): New macro... (em-tramp-test/su-default, em-tramp-test/su-user) (em-tramp-test/su-login, em-tramp-test/sudo-shell) (em-tramp-test/sudo-user-shell, em-tramp-test/doas-shell) (em-tramp-test/doas-user-shell): ... use it. --- lisp/eshell/esh-cmd.el | 25 +++-- lisp/eshell/esh-io.el | 123 ++++++++++++++--------- test/lisp/eshell/em-tramp-tests.el | 99 ++++++++---------- test/lisp/eshell/esh-io-tests.el | 23 ++++- test/lisp/eshell/eshell-tests-helpers.el | 29 ++++-- test/lisp/eshell/eshell-tests.el | 19 ++-- 6 files changed, 180 insertions(+), 138 deletions(-) diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 79957aeb416..6ba4ee41e70 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -419,11 +419,10 @@ eshell-parse-command (let ((cmd commands)) (while cmd ;; Copy I/O handles so each full statement can manipulate them - ;; if they like. As a small optimization, skip this for the - ;; last top-level one; we won't use these handles again - ;; anyway. - (when (or (not toplevel) (cdr cmd)) - (setcar cmd `(eshell-with-copied-handles ,(car cmd)))) + ;; if they like. Skip this for the last command in the list + ;; though; we won't use these handles again anyway. + (setcar cmd `(eshell-with-copied-handles + ,(car cmd) ,(not (cdr cmd)))) (setq cmd (cdr cmd)))) (if toplevel `(eshell-commands (progn @@ -792,10 +791,12 @@ eshell-trap-errors (defvar eshell-output-handle) ;Defined in esh-io.el. (defvar eshell-error-handle) ;Defined in esh-io.el. -(defmacro eshell-with-copied-handles (object) - "Duplicate current I/O handles, so OBJECT works with its own copy." +(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))) + (eshell-duplicate-handles eshell-current-handles ,steal-p))) ,object)) (define-obsolete-function-alias 'eshell-copy-handles @@ -836,7 +837,9 @@ eshell-do-pipelines (let ((proc ,(car pipeline))) (set headproc (or proc (symbol-value headproc))) (set tailproc (or (symbol-value tailproc) proc)) - proc)))))) + proc))) + ;; Steal handles if this is the last item in the pipeline. + ,(null (cdr pipeline))))) (defmacro eshell-do-pipelines-synchronously (pipeline) "Execute the commands in PIPELINE in sequence synchronously. @@ -1024,7 +1027,9 @@ eshell-eval-command ;; We can just stick the new command at the end of the current ;; one, and everything will happen as it should. (setcdr (last (cdr eshell-current-command)) - (list `(let ((here (and (eobp) (point)))) + (list `(let ((here (and (eobp) (point))) + (eshell-command-body '(nil)) + (eshell-test-body '(nil))) ,(and input `(insert-and-inherit ,(concat input "\n"))) (if here diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el index f2bc87374c1..90826a312b3 100644 --- a/lisp/eshell/esh-io.el +++ b/lisp/eshell/esh-io.el @@ -302,35 +302,51 @@ eshell-create-handles The result is a vector of file handles. Each handle is of the form: - (TARGETS DEFAULT REF-COUNT) + ((TARGETS . REF-COUNT) DEFAULT) -TARGETS is a list of destinations for output. DEFAULT is non-nil -if handle has its initial default value (always t after calling -this function). REF-COUNT is the number of references to this -handle (initially 1); see `eshell-protect-handles' and -`eshell-close-handles'." +TARGETS is a list of destinations for output. REF-COUNT is the +number of references to this handle (initially 1); see +`eshell-protect-handles' and `eshell-close-handles'. DEFAULT is +non-nil if handle has its initial default value (always t after +calling this function)." (let* ((handles (make-vector eshell-number-of-handles nil)) - (output-target (eshell-get-targets stdout output-mode)) - (error-target (if stderr - (eshell-get-targets stderr error-mode) - output-target))) - (aset handles eshell-output-handle (list output-target t 1)) - (aset handles eshell-error-handle (list error-target t 1)) + (output-target + (let ((target (eshell-get-target stdout output-mode))) + (cons (when target (list target)) 1))) + (error-target + (if stderr + (let ((target (eshell-get-target stderr error-mode))) + (cons (when target (list target)) 1)) + (cl-incf (cdr output-target)) + output-target))) + (aset handles eshell-output-handle (list output-target t)) + (aset handles eshell-error-handle (list error-target t)) handles)) -(defun eshell-duplicate-handles (handles) +(defun eshell-duplicate-handles (handles &optional steal-p) "Create a duplicate of the file handles in HANDLES. -This will copy the targets of each handle in HANDLES, setting the -DEFAULT field to t (see `eshell-create-handles')." - (eshell-create-handles - (car (aref handles eshell-output-handle)) nil - (car (aref handles eshell-error-handle)) nil)) +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'). + +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." + (let ((dup-handles (make-vector eshell-number-of-handles nil))) + (dotimes (idx eshell-number-of-handles) + (when-let ((handle (aref handles idx))) + (unless steal-p + (cl-incf (cdar handle))) + (aset dup-handles idx (list (car handle) t)))) + dup-handles)) (defun eshell-protect-handles (handles) "Protect the handles in HANDLES from a being closed." (dotimes (idx eshell-number-of-handles) (when-let ((handle (aref handles idx))) - (setcar (nthcdr 2 handle) (1+ (nth 2 handle))))) + (cl-incf (cdar handle)))) handles) (defun eshell-close-handles (&optional exit-code result handles) @@ -348,29 +364,45 @@ eshell-close-handles (when result (cl-assert (eq (car result) 'quote)) (setq eshell-last-command-result (cadr result))) - (let ((handles (or handles eshell-current-handles))) + (let ((handles (or handles eshell-current-handles)) + (succeeded (= eshell-last-command-status 0))) (dotimes (idx eshell-number-of-handles) - (when-let ((handle (aref handles idx))) - (setcar (nthcdr 2 handle) (1- (nth 2 handle))) - (when (= (nth 2 handle) 0) - (dolist (target (ensure-list (car (aref handles idx)))) - (eshell-close-target target (= eshell-last-command-status 0))) - (setcar handle nil)))))) + (eshell-close-handle (aref handles idx) succeeded)))) + +(defun eshell-close-handle (handle status) + "Close a single HANDLE, taking refcounts into account. +This will pass STATUS to each target for the handle, which should +be a non-nil value on successful termination." + (when handle + (cl-assert (> (cdar handle) 0) + "Attempted to close a handle with 0 references") + (when (and (> (cdar handle) 0) + (= (cl-decf (cdar handle)) 0)) + (dolist (target (caar handle)) + (eshell-close-target target status)) + (setcar (car handle) nil)))) (defun eshell-set-output-handle (index mode &optional target handles) "Set handle INDEX for the current HANDLES to point to TARGET using MODE. -If HANDLES is nil, use `eshell-current-handles'." +If HANDLES is nil, use `eshell-current-handles'. + +If the handle is currently set to its default value (see +`eshell-create-handles'), this will overwrite the targets with +the new target. Otherwise, it will append the new target to the +current list of targets." (when target (let* ((handles (or handles eshell-current-handles)) (handle (or (aref handles index) - (aset handles index (list nil nil 1)))) - (defaultp (cadr handle)) - (current (unless defaultp (car handle)))) + (aset handles index (list (cons nil 1) nil)))) + (defaultp (cadr handle))) + (when defaultp + (cl-decf (cdar handle)) + (setcar handle (cons nil 1))) (catch 'eshell-null-device - (let ((where (eshell-get-target target mode))) + (let ((current (caar handle)) + (where (eshell-get-target target mode))) (unless (member where current) - (setq current (append current (list where)))))) - (setcar handle current) + (setcar (car handle) (append current (list where)))))) (setcar (cdr handle) nil)))) (defun eshell-copy-output-handle (index index-to-copy &optional handles) @@ -378,10 +410,10 @@ eshell-copy-output-handle If HANDLES is nil, use `eshell-current-handles'." (let* ((handles (or handles eshell-current-handles)) (handle-to-copy (car (aref handles index-to-copy)))) - (setcar (aref handles index) - (if (listp handle-to-copy) - (copy-sequence handle-to-copy) - handle-to-copy)))) + (when handle-to-copy + (cl-incf (cdr handle-to-copy))) + (eshell-close-handle (aref handles index) nil) + (setcar (aref handles index) handle-to-copy))) (defun eshell-set-all-output-handles (mode &optional target handles) "Set output and error HANDLES to point to TARGET using MODE. @@ -501,13 +533,6 @@ eshell-get-target (error "Invalid redirection target: %s" (eshell-stringify target))))) -(defun eshell-get-targets (targets &optional mode) - "Convert TARGETS into valid output targets. -TARGETS can be a single raw target or a list thereof. MODE is either -`overwrite', `append' or `insert'; if it is omitted or nil, it -defaults to `insert'." - (mapcar (lambda (i) (eshell-get-target i mode)) (ensure-list targets))) - (defun eshell-interactive-output-p (&optional index handles) "Return non-nil if the specified handle is bound for interactive display. HANDLES is the set of handles to check; if nil, use @@ -519,9 +544,9 @@ eshell-interactive-output-p (let ((handles (or handles eshell-current-handles)) (index (or index eshell-output-handle))) (if (eq index 'all) - (and (equal (car (aref handles eshell-output-handle)) '(t)) - (equal (car (aref handles eshell-error-handle)) '(t))) - (equal (car (aref handles index)) '(t))))) + (and (equal (caar (aref handles eshell-output-handle)) '(t)) + (equal (caar (aref handles eshell-error-handle)) '(t))) + (equal (caar (aref handles index)) '(t))))) (defvar eshell-print-queue nil) (defvar eshell-print-queue-count -1) @@ -628,8 +653,8 @@ eshell-output-object If HANDLE-INDEX is nil, output to `eshell-output-handle'. HANDLES is the set of file handles to use; if nil, use `eshell-current-handles'." - (let ((targets (car (aref (or handles eshell-current-handles) - (or handle-index eshell-output-handle))))) + (let ((targets (caar (aref (or handles eshell-current-handles) + (or handle-index eshell-output-handle))))) (dolist (target targets) (eshell-output-object-to-target object target)))) diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el index 982a1eba279..936397d8869 100644 --- a/test/lisp/eshell/em-tramp-tests.el +++ b/test/lisp/eshell/em-tramp-tests.el @@ -23,40 +23,41 @@ (require 'em-tramp) (require 'tramp) +(defmacro em-tramp-test/should-replace-command (form replacement) + "Check that calling FORM results in it being replaced with REPLACEMENT." + (declare (indent 1)) + `(should (equal + (catch 'eshell-replace-command ,form) + (list 'eshell-with-copied-handles + (list 'eshell-trap-errors + ,replacement) + t)))) + (ert-deftest em-tramp-test/su-default () "Test Eshell `su' command with no arguments." - (should (equal - (catch 'eshell-replace-command (eshell/su)) - `(eshell-with-copied-handles - (eshell-trap-errors - (eshell-named-command - "cd" - (list ,(format "/su:root@%s:%s" - tramp-default-host default-directory)))))))) + (em-tramp-test/should-replace-command (eshell/su) + `(eshell-named-command + "cd" + (list ,(format "/su:root@%s:%s" + tramp-default-host default-directory))))) (ert-deftest em-tramp-test/su-user () "Test Eshell `su' command with USER argument." - (should (equal - (catch 'eshell-replace-command (eshell/su "USER")) - `(eshell-with-copied-handles - (eshell-trap-errors - (eshell-named-command - "cd" - (list ,(format "/su:USER@%s:%s" - tramp-default-host default-directory)))))))) + (em-tramp-test/should-replace-command (eshell/su "USER") + `(eshell-named-command + "cd" + (list ,(format "/su:USER@%s:%s" + tramp-default-host default-directory))))) (ert-deftest em-tramp-test/su-login () "Test Eshell `su' command with -/-l/--login option." (dolist (args '(("--login") ("-l") ("-"))) - (should (equal - (catch 'eshell-replace-command (apply #'eshell/su args)) - `(eshell-with-copied-handles - (eshell-trap-errors - (eshell-named-command - "cd" - (list ,(format "/su:root@%s:~/" tramp-default-host))))))))) + (em-tramp-test/should-replace-command (apply #'eshell/su args) + `(eshell-named-command + "cd" + (list ,(format "/su:root@%s:~/" tramp-default-host)))))) (defun mock-eshell-named-command (&rest args) "Dummy function to test Eshell `sudo' command rewriting." @@ -92,25 +93,19 @@ em-tramp-test/sudo-shell "Test Eshell `sudo' command with -s/--shell option." (dolist (args '(("--shell") ("-s"))) - (should (equal - (catch 'eshell-replace-command (apply #'eshell/sudo args)) - `(eshell-with-copied-handles - (eshell-trap-errors - (eshell-named-command - "cd" - (list ,(format "/sudo:root@%s:%s" - tramp-default-host default-directory))))))))) + (em-tramp-test/should-replace-command (apply #'eshell/sudo args) + `(eshell-named-command + "cd" + (list ,(format "/sudo:root@%s:%s" + tramp-default-host default-directory)))))) (ert-deftest em-tramp-test/sudo-user-shell () "Test Eshell `sudo' command with -s and -u options." - (should (equal - (catch 'eshell-replace-command (eshell/sudo "-u" "USER" "-s")) - `(eshell-with-copied-handles - (eshell-trap-errors - (eshell-named-command - "cd" - (list ,(format "/sudo:USER@%s:%s" - tramp-default-host default-directory)))))))) + (em-tramp-test/should-replace-command (eshell/sudo "-u" "USER" "-s") + `(eshell-named-command + "cd" + (list ,(format "/sudo:USER@%s:%s" + tramp-default-host default-directory))))) (ert-deftest em-tramp-test/doas-basic () "Test Eshell `doas' command with default user." @@ -147,24 +142,18 @@ em-tramp-test/doas-shell "Test Eshell `doas' command with -s/--shell option." (dolist (args '(("--shell") ("-s"))) - (should (equal - (catch 'eshell-replace-command (apply #'eshell/doas args)) - `(eshell-with-copied-handles - (eshell-trap-errors - (eshell-named-command - "cd" - (list ,(format "/doas:root@%s:%s" - tramp-default-host default-directory))))))))) + (em-tramp-test/should-replace-command (apply #'eshell/doas args) + `(eshell-named-command + "cd" + (list ,(format "/doas:root@%s:%s" + tramp-default-host default-directory)))))) (ert-deftest em-tramp-test/doas-user-shell () "Test Eshell `doas' command with -s and -u options." - (should (equal - (catch 'eshell-replace-command (eshell/doas "-u" "USER" "-s")) - `(eshell-with-copied-handles - (eshell-trap-errors - (eshell-named-command - "cd" - (list ,(format "/doas:USER@%s:%s" - tramp-default-host default-directory)))))))) + (em-tramp-test/should-replace-command (eshell/doas "-u" "USER" "-s") + `(eshell-named-command + "cd" + (list ,(format "/doas:USER@%s:%s" + tramp-default-host default-directory))))) ;;; em-tramp-tests.el ends here diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el index 9a3c14f365f..0f09afa19e4 100644 --- a/test/lisp/eshell/esh-io-tests.el +++ b/test/lisp/eshell/esh-io-tests.el @@ -301,15 +301,28 @@ esh-io-test/redirect-copy-first "stderr\n")) (should (equal (buffer-string) "stdout\n")))) -(ert-deftest esh-io-test/redirect-pipe () - "Check that \"redirecting\" to a pipe works." - ;; `|' should only redirect stdout. + +;; Pipelines + +(ert-deftest esh-io-test/pipeline/default () + "Check that `|' only pipes stdout." + (skip-unless (executable-find "rev")) (eshell-command-result-equal "test-output | rev" - "stderr\ntuodts\n") - ;; `|&' should redirect stdout and stderr. + "stderr\ntuodts\n")) + + +(ert-deftest esh-io-test/pipeline/all () + "Check that `|&' only pipes stdout and stderr." + (skip-unless (executable-find "rev")) (eshell-command-result-equal "test-output |& rev" "tuodts\nrredts\n")) +(ert-deftest esh-io-test/pipeline/subcommands () + "Chek that all commands in a subcommand are properly piped." + (skip-unless (executable-find "rev")) + (eshell-command-result-equal "{echo foo; echo bar} | rev" + "raboof")) + ;; Virtual targets diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el index 1d9674070c0..a9338050311 100644 --- a/test/lisp/eshell/eshell-tests-helpers.el +++ b/test/lisp/eshell/eshell-tests-helpers.el @@ -33,9 +33,9 @@ (defvar eshell-history-file-name nil) (defvar eshell-last-dir-ring-file-name nil) -(defvar eshell-test--max-subprocess-time 5 - "The maximum amount of time to wait for a subprocess to finish, in seconds. -See `eshell-wait-for-subprocess'.") +(defvar eshell-test--max-wait-time 5 + "The maximum amount of time to wait for a condition to resolve, in seconds. +See `eshell-wait-for'.") (defun eshell-tests-remote-accessible-p () "Return if a test involving remote files can proceed. @@ -73,19 +73,28 @@ eshell-with-temp-buffer (let ((,bufname (buffer-name))) ,@body))) +(defun eshell-wait-for (predicate &optional message) + "Wait until PREDICATE returns non-nil. +If this takes longer than `eshell-test--max-wait-time', raise an +error. MESSAGE is an optional message to use if this times out." + (let ((start (current-time)) + (message (or message "timed out waiting for condition"))) + (while (not (funcall predicate)) + (when (> (float-time (time-since start)) + eshell-test--max-wait-time) + (error message)) + (sit-for 0.1)))) + (defun eshell-wait-for-subprocess (&optional all) "Wait until there is no interactive subprocess running in Eshell. If ALL is non-nil, wait until there are no Eshell subprocesses at all running. -If this takes longer than `eshell-test--max-subprocess-time', +If this takes longer than `eshell-test--max-wait-time', raise an error." - (let ((start (current-time))) - (while (if all eshell-process-list (eshell-interactive-process-p)) - (when (> (float-time (time-since start)) - eshell-test--max-subprocess-time) - (error "timed out waiting for subprocess(es)")) - (sit-for 0.1)))) + (eshell-wait-for + (lambda () + (not (if all eshell-process-list (eshell-interactive-process-p)))))) (defun eshell-insert-command (command &optional func) "Insert a COMMAND at the end of the buffer. diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el index c67ac67fd36..dd8be8e65f0 100644 --- a/test/lisp/eshell/eshell-tests.el +++ b/test/lisp/eshell/eshell-tests.el @@ -128,16 +128,17 @@ eshell-test/forward-arg (delete-region (point) (point-max)))))) (ert-deftest eshell-test/queue-input () - "Test queuing command input" + "Test queuing command input. +This should let the current command finish, then automatically +insert the queued one at the next prompt, and finally run it." (with-temp-eshell - (eshell-insert-command "sleep 2") - (eshell-insert-command "echo alpha" 'eshell-queue-input) - (let ((count 10)) - (while (and eshell-current-command - (> count 0)) - (sit-for 1) - (setq count (1- count)))) - (should (eshell-match-output "alpha\n")))) + (eshell-insert-command "sleep 1; echo slept") + (eshell-insert-command "echo alpha" #'eshell-queue-input) + (let ((start (marker-position (eshell-beginning-of-output)))) + (eshell-wait-for (lambda () (not eshell-current-command))) + (should (string-match "^slept\n.*echo alpha\nalpha\n$" + (buffer-substring-no-properties + start (eshell-end-of-output))))))) (ert-deftest eshell-test/flush-output () "Test flushing of previous output" -- 2.25.1