master 073da412a1: Fix reference-counting of Eshell I/O handles

[Jim Porter]

branch: master
commit 073da412a139e317959f56e359ed12de726a0a35
Author: Jim Porter <jporterbugs@gmail.com>
Commit: Jim Porter <jporterbugs@gmail.com>

    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/esh-cmd-tests.el (esh-cmd-test/for-loop-pipe)
    (esh-cmd-test/while-loop-pipe, esh-cmd-test/if-statement-pipe)
    esh-cmd-test/if-else-statement-pipe): New tests.
    (esh-cmd-test/while-loop): Use 'pop' to simplify the test a bit.
    
    * 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-cmd-tests.el        |  44 ++++++++++-
 test/lisp/eshell/esh-io-tests.el         |  23 ++++--
 test/lisp/eshell/eshell-tests-helpers.el |  29 +++++---
 test/lisp/eshell/eshell-tests.el         |  19 ++---
 7 files changed, 222 insertions(+), 140 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 79957aeb41..39579335cf 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -419,11 +419,10 @@ hooks should be run before and after the 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.  Steal the handles for the last command in
+        ;; the list; we won't use the originals 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 @@ this grossness will be made to disappear by using 
`call/cc'..."
 (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 @@ This macro calls itself recursively, with NOTFIRST non-nil."
           (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 @@ produced by `eshell-parse-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 f2bc87374c..90826a312b 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -302,35 +302,51 @@ value of mode defaults to `insert'.
 
 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 @@ the value already set in `eshell-last-command-result'."
   (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 @@ If HANDLES is nil, use `eshell-current-handles'."
 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 @@ it defaults to `insert'."
     (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 @@ INDEX is the handle index to check.  If nil, check
   (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 @@ Returns what was actually sent, or nil if nothing was sent."
 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 982a1eba27..936397d886 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 @@
   "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 @@
   "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-cmd-tests.el 
b/test/lisp/eshell/esh-cmd-tests.el
index 92d785d7fd..cc40dde355 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -148,14 +148,21 @@ e.g. \"{(+ 1 2)} 3\" => 3"
       "echo $name; for name in 3 { echo $name }; echo $name"
       "env-value\n3\nenv-value\n"))))
 
+(ert-deftest esh-cmd-test/for-loop-pipe ()
+  "Test invocation of a for loop piped to another command."
+  (skip-unless (executable-find "rev"))
+  (with-temp-eshell
+   (eshell-match-command-output "for i in foo bar baz { echo $i } | rev"
+                                "zabraboof")))
+
 (ert-deftest esh-cmd-test/while-loop ()
   "Test invocation of a while loop."
   (with-temp-eshell
    (let ((eshell-test-value '(0 1 2)))
      (eshell-match-command-output
       (concat "while $eshell-test-value "
-              "{ setq eshell-test-value (cdr eshell-test-value) }")
-      "(1 2)\n(2)\n"))))
+              "{ (pop eshell-test-value) }")
+      "0\n1\n2\n"))))
 
 (ert-deftest esh-cmd-test/while-loop-lisp-form ()
   "Test invocation of a while loop using a Lisp form."
@@ -176,6 +183,17 @@ e.g. \"{(+ 1 2)} 3\" => 3"
               "{ setq eshell-test-value (1+ eshell-test-value) }")
       "1\n2\n3\n"))))
 
+(ert-deftest esh-cmd-test/while-loop-pipe ()
+  "Test invocation of a while loop piped to another command."
+  (skip-unless (executable-find "rev"))
+  (with-temp-eshell
+   (let ((eshell-test-value '("foo" "bar" "baz")))
+     (eshell-match-command-output
+      (concat "while $eshell-test-value "
+              "{ (pop eshell-test-value) }"
+              " | rev")
+      "zabraboof"))))
+
 (ert-deftest esh-cmd-test/until-loop ()
   "Test invocation of an until loop."
   (with-temp-eshell
@@ -253,6 +271,28 @@ This tests when `eshell-lisp-form-nil-is-failure' is nil."
   (eshell-command-result-equal "if {[ foo = bar ]} {echo yes} {echo no}"
                                "no"))
 
+(ert-deftest esh-cmd-test/if-statement-pipe ()
+  "Test invocation of an if statement piped to another command."
+  (skip-unless (executable-find "rev"))
+  (let ((eshell-test-value t))
+    (eshell-command-result-equal "if $eshell-test-value {echo yes} | rev"
+                                 "sey"))
+  (let ((eshell-test-value nil))
+    (eshell-command-result-equal "if $eshell-test-value {echo yes} | rev"
+                                 nil)))
+
+(ert-deftest esh-cmd-test/if-else-statement-pipe ()
+  "Test invocation of an if/else statement piped to another command."
+  (skip-unless (executable-find "rev"))
+  (let ((eshell-test-value t))
+    (eshell-command-result-equal
+     "if $eshell-test-value {echo yes} {echo no} | rev"
+     "sey"))
+  (let ((eshell-test-value nil))
+    (eshell-command-result-equal
+     "if $eshell-test-value {echo yes} {echo no} | rev"
+     "on")))
+
 (ert-deftest esh-cmd-test/unless-statement ()
   "Test invocation of an unless statement."
   (let ((eshell-test-value t))
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index 9a3c14f365..0f09afa19e 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -301,15 +301,28 @@ stdout originally pointed (the terminal)."
                                   "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 1d9674070c..a933805031 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 @@ BUFNAME will be set to the name of the temporary 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 c67ac67fd3..dd8be8e65f 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -128,16 +128,17 @@
        (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"