>From 5a144e547d6bd6a718e76499705d81ef5934776d Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Tue, 15 Jan 2019 10:18:45 -0800 Subject: [PATCH] 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 72b164c5d4..afda8aede8 100644 --- a/doc/lispref/processes.texi +++ b/doc/lispref/processes.texi @@ -1859,6 +1859,26 @@ Accepting Output 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 e2275bee2a..ca47601e4b 100644 --- a/lisp/net/tramp-adb.el +++ b/lisp/net/tramp-adb.el @@ -206,9 +206,9 @@ tramp-adb-parse-device-names (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 0aa110d9a4..7366057296 100644 --- a/lisp/net/tramp-rclone.el +++ b/lisp/net/tramp-rclone.el @@ -183,9 +183,9 @@ tramp-rclone-parse-device-names (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 0c42a5d1a5..abf3248a35 100644 --- a/lisp/net/tramp-smb.el +++ b/lisp/net/tramp-smb.el @@ -2038,10 +2038,10 @@ tramp-smb-wait-for-output ;; 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 @@ tramp-smb-wait-for-output (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 cab9b8d835..e1e5ab091a 100644 --- a/lisp/net/tramp-sudoedit.el +++ b/lisp/net/tramp-sudoedit.el @@ -746,6 +746,8 @@ tramp-sudoedit-handle-write-region (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 437e2d19b9..7632d656a0 100644 --- a/lisp/net/tramp.el +++ b/lisp/net/tramp.el @@ -3977,6 +3977,8 @@ tramp-action-process-alive (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 @@ tramp-interrupt-process ;; 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 514bd04da4..5dbf441e8c 100644 --- a/test/src/process-tests.el +++ b/test/src/process-tests.el @@ -207,8 +207,8 @@ process-tests--mixable :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)) -- 2.20.1