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.

diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
index 72b164c5d455a764cc281c28b4346dc77534c944..afda8aede83e6d806f1e59602a286adec399a958 100644 (file)
--- 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 e2275bee2a4179f6524d7f8c233c7b050d49cfb3..ca47601e4bd1a4277665a57f382b0237eec555db 100644 (file)
--- 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 0aa110d9a4685e7fb48addc4e438768b15e48207..736605729667f07d1626379dbc7b1018203f5be7 100644 (file)
--- 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 0c42a5d1a538a71fe5b67df84513448bbcbe8446..abf3248a35341870fe802cbe93968904885c74b4 100644 (file)
--- 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 cab9b8d835a2c86b28366376fc18b0ecc95f4923..e1e5ab091a1b42d18ae0c37f016a4ad805f06acf 100644 (file)
--- 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 437e2d19b9ec3fda58bcb340a91436307cdfa5e6..7632d656a0f7df881beb87d4a4b36365e26894cb 100644 (file)
--- 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 514bd04da4e5b2da2e513e34fed39b4068ce1006..5dbf441e8c2afb23676541ca800062016b5ff5ad 100644 (file)
--- 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))