Commit 9fc02ff5 authored by Paul Eggert's avatar Paul Eggert
Browse files

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.
parent 223e7b87
Pipeline #533 failed with stage
in 45 minutes and 50 seconds
......@@ -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
......
......@@ -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)
......
......@@ -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)
......
......@@ -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))
......
......@@ -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.
......
......@@ -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)))))
......
......@@ -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))
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment