[Emacs-diffs] master 9fc02ff: Fix accept-process-output/process-live-p confusion

[Paul Eggert]

branch: master
commit 9fc02ff5ea95c31a8d81eabb5634aa135fcd8786
Author: Paul Eggert <address@hidden>
Commit: Paul Eggert <address@hidden>

    Fix accept-process-output/process-live-p confusion
    
    * doc/lispref/processes.texi (Accepting Output):
    Document the issue.
    * lisp/net/tramp-adb.el (tramp-adb-parse-device-names):
    * lisp/net/tramp-rclone.el (tramp-rclone-parse-device-names):
    * lisp/net/tramp-smb.el (tramp-smb-wait-for-output):
    * lisp/net/tramp.el (tramp-interrupt-process):
    * test/src/process-tests.el (make-process/mix-stderr):
    Fix code that uses accept-process-output and process-live-p.
    Add FIXME comments as necessary.
    * lisp/net/tramp-sudoedit.el (tramp-sudoedit-action-sudo):
    * lisp/net/tramp.el (tramp-action-out-of-band):
    Add FIXME comments as necessary.
---
 doc/lispref/processes.texi | 20 ++++++++++++++++++++
 lisp/net/tramp-adb.el      |  6 +++---
 lisp/net/tramp-rclone.el   |  6 +++---
 lisp/net/tramp-smb.el      | 19 +++++++++++--------
 lisp/net/tramp-sudoedit.el |  2 ++
 lisp/net/tramp.el          |  9 ++++++---
 test/src/process-tests.el  |  4 ++--
 7 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
index 72b164c..afda8ae 100644
--- a/doc/lispref/processes.texi
+++ b/doc/lispref/processes.texi
@@ -1859,6 +1859,26 @@ corresponding connection contains buffered data.  The 
function returns
 arrived.
 @end defun
 
+If a connection from a process contains buffered data,
address@hidden can return address@hidden even after the
+process has exited.  Therefore, although the following loop:
+
address@hidden
+;; This loop contains a bug.
+(while (process-live-p process)
+  (accept-process-output process))
address@hidden example
+
address@hidden
+will often work, it has a race condition and can miss some output if
address@hidden returns @code{nil} while the connection still
+contains data.  Better is to write the loop like this:
+
address@hidden
+(while (or (accept-process-output process)
+           (process-live-p process)))
address@hidden example
+
 @node Processes and Threads
 @subsection Processes and Threads
 @cindex processes, threads
diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index e2275be..ca47601 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -206,9 +206,9 @@ pass to the OPERATION."
        (tramp-message v 6 "%s" (mapconcat 'identity (process-command p) " "))
        (process-put p 'adjust-window-size-function 'ignore)
        (set-process-query-on-exit-flag p nil)
-       (while (process-live-p p)
-         (accept-process-output p 0.1))
-       (accept-process-output p 0.1)
+       ;; FIXME: Either remove " 0.1", or comment why it's needed.
+       (while (or (accept-process-output p 0.1)
+                  (process-live-p p)))
        (tramp-message v 6 "\n%s" (buffer-string))
        (goto-char (point-min))
        (while (search-forward-regexp "^\\(\\S-+\\)[[:space:]]+device$" nil t)
diff --git a/lisp/net/tramp-rclone.el b/lisp/net/tramp-rclone.el
index 0aa110d..7366057 100644
--- a/lisp/net/tramp-rclone.el
+++ b/lisp/net/tramp-rclone.el
@@ -183,9 +183,9 @@ pass to the OPERATION."
          (tramp-message v 6 "%s" (mapconcat 'identity (process-command p) " "))
          (process-put p 'adjust-window-size-function 'ignore)
          (set-process-query-on-exit-flag p nil)
-         (while (process-live-p p)
-           (accept-process-output p 0.1))
-         (accept-process-output p 0.1)
+         ;; FIXME: Either remove " 0.1", or comment why it's needed.
+         (while (or (accept-process-output p 0.1)
+                    (process-live-p p)))
          (tramp-message v 6 "\n%s" (buffer-string))
          (goto-char (point-min))
          (while (search-forward-regexp "^\\(\\S-+\\):$" nil t)
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index 0c42a5d..abf3248 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -2038,10 +2038,10 @@ Returns nil if an error message has appeared."
       ;; Algorithm: get waiting output.  See if last line contains
       ;; `tramp-smb-prompt' sentinel or `tramp-smb-errors' strings.
       ;; If not, wait a bit and again get waiting output.
-      (while (and (not found) (not err) (process-live-p p))
-
-       ;; Accept pending output.
-       (tramp-accept-process-output p 0.1)
+      ;; FIXME: Either remove " 0.1", or comment why it's needed.
+      (while (and (not found) (not err)
+                 (or (tramp-accept-process-output p 0.1)
+                     (process-live-p p)))
 
        ;; Search for prompt.
        (goto-char (point-min))
@@ -2052,10 +2052,13 @@ Returns nil if an error message has appeared."
        (setq err (re-search-forward tramp-smb-errors nil t)))
 
       ;; When the process is still alive, read pending output.
-      (while (and (not found) (process-live-p p))
-
-       ;; Accept pending output.
-       (tramp-accept-process-output p 0.1)
+      ;; FIXME: This loop should be folded into the previous loop.
+      ;; Also, ERR should be set just once, after the combined
+      ;; loop has finished.
+      ;; FIXME: Either remove " 0.1", or comment why it's needed.
+      (while (and (not found)
+                 (or (tramp-accept-process-output p 0.1)
+                     (process-live-p p)))
 
        ;; Search for prompt.
        (goto-char (point-min))
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index cab9b8d..e1e5ab0 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -746,6 +746,8 @@ ID-FORMAT valid values are `string' and `integer'."
 (defun tramp-sudoedit-action-sudo (proc vec)
   "Check, whether a sudo process copy has finished."
   ;; There might be pending output for the exit status.
+  ;; FIXME: Either remove " 0.1", or comment why it's needed.
+  ;; FIXME: There's a race here.  Shouldn't the next two lines be interchanged?
   (tramp-accept-process-output proc 0.1)
   (when (not (process-live-p proc))
     ;; Delete narrowed region, it would be in the way reading a Lisp form.
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 437e2d1..7632d65 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -3977,6 +3977,8 @@ The terminal type can be configured with 
`tramp-terminal-type'."
 (defun tramp-action-out-of-band (proc vec)
   "Check, whether an out-of-band copy has finished."
   ;; There might be pending output for the exit status.
+  ;; FIXME: Either remove " 0.1", or comment why it's needed.
+  ;; FIXME: Shouldn't the following line be wrapped inside (while ...)?
   (tramp-accept-process-output proc 0.1)
   (cond ((and (not (process-live-p proc))
              (zerop (process-exit-status proc)))
@@ -4821,9 +4823,10 @@ Only works for Bourne-like shells."
        ;; Wait, until the process has disappeared.  If it doesn't,
        ;; fall back to the default implementation.
        (with-timeout (1 (ignore))
-         (while (process-live-p proc)
-           ;; We cannot run `tramp-accept-process-output', it blocks timers.
-           (accept-process-output proc 0.1))
+         ;; We cannot run `tramp-accept-process-output', it blocks timers.
+         ;; FIXME: Either remove " 0.1", or comment why it's needed.
+         (while (or (accept-process-output proc 0.1)
+                    (process-live-p proc)))
          ;; Report success.
          proc)))))
 
diff --git a/test/src/process-tests.el b/test/src/process-tests.el
index 514bd04..5dbf441 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -207,8 +207,8 @@
                     :sentinel #'ignore
                     :noquery t
                     :connection-type 'pipe)))
-      (while (process-live-p process)
-        (accept-process-output process))
+      (while (or (accept-process-output process)
+                (process-live-p process)))
       (should (eq (process-status process) 'exit))
       (should (eq (process-exit-status process) 0))
       (should (process-tests--mixable (string-to-list (buffer-string))