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.

7 files changed, 47 insertions, 19 deletions

diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
index 72b164c5d45..afda8aede83 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,
+@code{accept-process-output} can return non-@code{nil} even after the
+process has exited.  Therefore, although the following loop:
+
+@example
+;; This loop contains a bug.
+(while (process-live-p process)
+  (accept-process-output process))
+@end example
+
+@noindent
+will often work, it has a race condition and can miss some output if
+@code{process-live-p} returns @code{nil} while the connection still
+contains data.  Better is to write the loop like this:
+
+@example
+(while (or (accept-process-output process)
+           (process-live-p process)))
+@end 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 e2275bee2a4..ca47601e4bd 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 0aa110d9a46..73660572966 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 0c42a5d1a53..abf3248a353 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 cab9b8d835a..e1e5ab091a1 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 437e2d19b9e..7632d656a0f 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 514bd04da4e..5dbf441e8c2 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))