GNU bug report logs - #71896
shell-resync-dirs hang

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Troy Hinckley <troyhinckley <at> dabrev.com>
Cc: 71896 <at> debbugs.gnu.org
Subject: bug#71896: shell-resync-dirs hang
Date: Sat, 06 Jul 2024 12:58:00 +0300

> Date: Mon, 1 Jul 2024 21:22:35 -0500
> From: Troy Hinckley <troyhinckley <at> dabrev.com>
> 
> This is related to #59804 and #54384.
> 
> I will occasionally (about 30% of the time) see hangs when running shell-resync-dirs in Emacs 29. I tracked this
> down to two issues:
> 
> First is in shell-eval-command. For Emacs 29 this function was added to fix #54384. It has this section in the
> code:
> 
> ```
> ;; Wait until we get a prompt (which will be a line without
>  ;; a newline). This is far from fool-proof -- if something
>  ;; outputs incomplete data and then sleeps, we'll think
>  ;; we've received the prompt.
>  (while (not (let* ((lines (string-lines result))
>  (last (car (last lines))))
>  (and (length> lines 0)
>  (not (equal last ""))
>  (or (not prev)
>  (not (equal last prev)))
>  (setq prev last))))
>  (accept-process-output proc 0 100))
> ```
> 
> Note that is says that is looking for “a line without a newline” to determine if we have reached the prompt.
> However this code does not actually do that. If the result ends in a newline it will still terminate the loop and not
> wait for more input. We can see that by the fact that the following code evaluates to nil.
> 
> ```
> (let ((result "dirs\n") prev)
>  (not (let* ((lines (string-lines result))
>  (last (car (last lines))))
>  (and (length> lines 0)
>  (not (equal last ""))
>  (or (not prev)
>  (not (equal last prev)))
>  (setq prev last)))))
> ```
> 
> I am not sure what this code is supposed to do, but the issue arrises if the process output sends anything to
> this function it will terminate and not wait for more input. In my case the issue is that the shell is echoing the
> command followed by the result (comint-process-echoes). About 30% of the time these two lines get sent as
> part of two different outputs. Meaning the second line (the directory for shell-resync-dirs) does not get captured
> and instead gets printed to the terminal.

Does the patch below solve the problem in shell-eval-command?

> This leads us to the hang. The issue is this code in shell-resync-dirs:
> 
> ```
> (while dlsl
>  (let ((newelt "")
>  tem1 tem2)
>  (while newelt
>  ;; We need tem1 because we don't want to prepend
>  ;; `comint-file-name-prefix' repeatedly into newelt via tem2.
>  (setq tem1 (pop dlsl)
>  tem2 (concat comint-file-name-prefix tem1 newelt))
>  (cond ((file-directory-p tem2)
>  (push tem2 ds)
>  (when (string= " " (car dlsl))
>  (pop dlsl))
>  (setq newelt nil))
>  (t
>  (setq newelt (concat tem1 newelt)))))))
> ```
> 
> This loop can only terminate if tem2 is a valid directory. Otherwise it will take the default case in the cond and
> loop forever. And since the bug in shell-eval-command does not provide the directory when the process output
> is split, we get a hang.
> 
> I believe both of these need to be fixed to properly fix the bug.
> 
> For the shell-eval-command I don’t understand what that loop is trying to do now, so I am not sure how to fix it
> without breaking its functionality. I would just use (string-suffix-p “\n” result) to check if the output ends in a
> newline, but the code is obviously trying to do something more complex there.
> 
> If we fix that issue then it will resolve the hang in shell-resync-dirs, but I think that is just glossing over the
> problem. That functions should never hang, no matter what output it get’s from the shell. My recommendation
> would be to add `(and dlsl newelt)` as the condition for the inner while loop with newelt. That way if dlsl is
> empty, it will terminate the loop since there is nothing more to process. This fixed the issue for me. 

Thanks, I think I agree with your suggestion for shell-resync-dirs.
But please undo the fix you evidently made there to avoid the infloop,
and see if the patch below for shell-eval-command makes
shell-resync-dirs do its job by correctly resynchronizing
shell-dirtrack.

diff --git a/lisp/shell.el b/lisp/shell.el
index e1936ff..f86156e 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -1629,10 +1629,12 @@ shell-eval-command
           ;; a newline).  This is far from fool-proof -- if something
           ;; outputs incomplete data and then sleeps, we'll think
           ;; we've received the prompt.
-          (while (not (let* ((lines (string-lines result))
-                             (last (car (last lines))))
+          (while (not (let* ((lines (string-lines result nil t))
+                             (last (car (last lines)))
+                             (last-end (substring last -1)))
                         (and (length> lines 0)
-                             (not (equal last ""))
+                             (not (member last '("" "\n")))
+                             (not (equal last-end "\n"))
                              (or (not prev)
                                  (not (equal last prev)))
                              (setq prev last))))

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.