all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: "João Távora" <joaotavora@gmail.com>
To: Michael Albinus <michael.albinus@gmx.de>
Cc: 62194@debbugs.gnu.org, miha@kamnitnik.top
Subject: bug#62194: 30.0.50; Two Eglot-over-Tramp tests are failing on master, passing on emacs-29
Date: Thu, 16 Mar 2023 15:12:29 +0000	[thread overview]
Message-ID: <87fsa43f36.fsf@gmail.com> (raw)
In-Reply-To: <87pm98iw1e.fsf@gmx.de> (Michael Albinus's message of "Thu, 16 Mar 2023 15:57:17 +0100")

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]

Michael Albinus <michael.albinus@gmx.de> writes:

> João Távora <joaotavora@gmail.com> writes:
>
> Hi João,
>
>> Michael, now you've brought back the Eglot/Tramp hang of bug#61350!
>>
>> Really Michael, please consider reverting 0330cff65ae (your latest) and
>> 54ef338ba36 (from two days ago), and going back to the simpler version
>> that you originally proposed, with just the 'while' added there.
>
> Honestly, I'm lost with all the patches back forth and back. Could you
> pls show me the change you propose, based on the current master state? 

OK.  Based on the current master state, I'm sending you 4 (four
patches).  I known it looks a lot, but is quite simple.

'0001-Revert-Michael-Albinus-Fix-regression-in-Tramp-bug-6.patch'

   Reverts your latest commit 0330cff65ae, where some settings of
   'shared-socket' are commented out.

'0002-Revert-Michael-Albinus-Improve-Tramp-processes-to-ac.patch'

   Reverts 54ef338ba36 from two days ago, for all purposes restoring the
   state of Tramp to what is in emacs-29.

'0003-A-simpler-fix-for-bug-61350-a-small-tweak-Michael-s-.patch'

   This is the simpler patch that adds the (while
   (accept-process-output)).  It is directly based on your first idea.
   It keeps the timeout.  It is the most conservative patch.

'0004-Just-for-testing-Remove-Tramp-specific-workaround-in.patch'

   This removes the Tramp-specific Eglot workaround.

   It's good for testing the original hang reporting in bug#61350

After you have applied these four patches, the following should happen:

* Tramp tests should have the same pass rate in master as in emacs-29

* bug#61350 should be fixed -- even without the Eglot workaround.  Let
  me know if you need help testing this (I have a nifty dockerfile and
  some command-line incantations that make testing very easy).

João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Revert-Michael-Albinus-Fix-regression-in-Tramp-bug-6.patch --]
[-- Type: text/x-patch, Size: 2829 bytes --]

From a168ef267aadf2d83627806c18853a7aaf1e88ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Thu, 16 Mar 2023 12:24:29 +0000
Subject: [PATCH 1/4] Revert Michael Albinus' "Fix regression in Tramp
 (bug#62194)"

This reverts commit 0330cff65ae837e93ae4d059acf643734d16386d.
---
 lisp/net/tramp-sh.el | 6 +++---
 lisp/net/tramp.el    | 3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index 2f990af334d..882b79b3ee7 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -2427,7 +2427,7 @@ tramp-do-copy-or-rename-file-out-of-band
 		;; This is neded for ssh or PuTTY based processes, and
 		;; only if the respective options are set.  Perhaps,
 		;; the setting could be more fine-grained.
-		;; (process-put p 'shared-socket t)
+		(process-put p 'shared-socket t)
 		(process-put p 'adjust-window-size-function #'ignore)
 		(set-process-query-on-exit-flag p nil)
 
@@ -3760,7 +3760,7 @@ tramp-sh-handle-file-notify-add-watch
 	;; This is neded for ssh or PuTTY based processes, and only if
 	;; the respective options are set.  Perhaps, the setting could
 	;; be more fine-grained.
-	;; (process-put p 'shared-socket t)
+	(process-put p 'shared-socket t)
 	;; Needed for process filter.
 	(process-put p 'events events)
 	(process-put p 'watch-name localname)
@@ -5124,7 +5124,7 @@ tramp-maybe-open-connection
 		;; This is neded for ssh or PuTTY based processes, and
 		;; only if the respective options are set.  Perhaps,
 		;; the setting could be more fine-grained.
-		;; (process-put p 'shared-socket t)
+		(process-put p 'shared-socket t)
 		(process-put p 'adjust-window-size-function #'ignore)
 		(set-process-query-on-exit-flag p nil)
 		(setq tramp-current-connection (cons vec (current-time)))
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 0c8f8acc07d..b6e985db6b1 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -5091,7 +5091,7 @@ tramp-handle-make-process
 	    ;; This is neded for ssh or PuTTY based processes, and
 	    ;; only if the respective options are set.  Perhaps, the
 	    ;; setting could be more fine-grained.
-	    ;; (process-put p 'shared-socket t)
+	    (process-put p 'shared-socket t)
 	    (process-put p 'remote-command orig-command)
 	    (tramp-set-connection-property p "remote-command" orig-command)
 
@@ -5809,7 +5809,6 @@ tramp-accept-process-output
   ;; There could be other processes which use the same socket for
   ;; communication.  This could block the output for the current
   ;; process.  Read such output first.  (Bug#61350)
-  ;; The process property isn't set anymore due to Bug#62194.
   (when-let (((process-get proc 'shared-socket))
 	     (v (process-get proc 'vector)))
     (dolist (p (delq proc (process-list)))
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Revert-Michael-Albinus-Improve-Tramp-processes-to-ac.patch --]
[-- Type: text/x-patch, Size: 9930 bytes --]

From 14e37318dd5f170771fb1a74d69e89d5e756d1cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Thu, 16 Mar 2023 12:26:04 +0000
Subject: [PATCH 2/4] Revert Michael Albinus' "Improve Tramp processes to
 accept..."

It was found to cause the failure of test
file-notify-test04-autorevert-remote, among other instabilities.

This reverts commit 54ef338ba3670415cf47fabc33a92d4904707c7e.
---
 lisp/net/tramp-adb.el      |  2 +-
 lisp/net/tramp-gvfs.el     |  2 +-
 lisp/net/tramp-sh.el       | 14 +-------------
 lisp/net/tramp-smb.el      |  6 +++---
 lisp/net/tramp-sudoedit.el |  2 +-
 lisp/net/tramp.el          | 33 +++++++++------------------------
 6 files changed, 16 insertions(+), 43 deletions(-)

diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index d338201ab72..64f45e7958d 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -990,7 +990,7 @@ tramp-adb-handle-make-process
 				  (progn
 				    (goto-char (point-min))
 				    (not (search-forward "\n" nil t)))
-			        (tramp-accept-process-output p))
+			        (tramp-accept-process-output p 0))
 			      (delete-region (point-min) (point)))
 			    ;; Provide error buffer.  This shows only
 			    ;; initial error messages; messages
diff --git a/lisp/net/tramp-gvfs.el b/lisp/net/tramp-gvfs.el
index c1ad37de1d2..266724c587f 100644
--- a/lisp/net/tramp-gvfs.el
+++ b/lisp/net/tramp-gvfs.el
@@ -1469,7 +1469,7 @@ tramp-gvfs-handle-file-notify-add-watch
 	(set-process-sentinel p #'tramp-file-notify-process-sentinel)
 	;; There might be an error if the monitor is not supported.
 	;; Give the filter a chance to read the output.
-	(while (tramp-accept-process-output p))
+	(while (tramp-accept-process-output p 0))
 	(unless (process-live-p p)
 	  (tramp-error
 	   p 'file-notify-error "Monitoring not supported for `%s'" file-name))
diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index 882b79b3ee7..48ebfff6cfe 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -2424,10 +2424,6 @@ tramp-do-copy-or-rename-file-out-of-band
 		      copy-program copy-args)))
 		(tramp-message v 6 "%s" (string-join (process-command p) " "))
 		(process-put p 'vector v)
-		;; This is neded for ssh or PuTTY based processes, and
-		;; only if the respective options are set.  Perhaps,
-		;; the setting could be more fine-grained.
-		(process-put p 'shared-socket t)
 		(process-put p 'adjust-window-size-function #'ignore)
 		(set-process-query-on-exit-flag p nil)
 
@@ -3757,10 +3753,6 @@ tramp-sh-handle-file-notify-add-watch
 	   (string-join sequence " "))
 	(tramp-message v 6 "Run `%s', %S" (string-join sequence " ") p)
 	(process-put p 'vector v)
-	;; This is neded for ssh or PuTTY based processes, and only if
-	;; the respective options are set.  Perhaps, the setting could
-	;; be more fine-grained.
-	(process-put p 'shared-socket t)
 	;; Needed for process filter.
 	(process-put p 'events events)
 	(process-put p 'watch-name localname)
@@ -3769,7 +3761,7 @@ tramp-sh-handle-file-notify-add-watch
 	(set-process-sentinel p #'tramp-file-notify-process-sentinel)
 	;; There might be an error if the monitor is not supported.
 	;; Give the filter a chance to read the output.
-	(while (tramp-accept-process-output p))
+	(while (tramp-accept-process-output p 0))
 	(unless (process-live-p p)
 	  (tramp-error
 	   p 'file-notify-error "Monitoring not supported for `%s'" file-name))
@@ -5121,10 +5113,6 @@ tramp-maybe-open-connection
 		;; Set sentinel and query flag.  Initialize variables.
 		(set-process-sentinel p #'tramp-process-sentinel)
 		(process-put p 'vector vec)
-		;; This is neded for ssh or PuTTY based processes, and
-		;; only if the respective options are set.  Perhaps,
-		;; the setting could be more fine-grained.
-		(process-put p 'shared-socket t)
 		(process-put p 'adjust-window-size-function #'ignore)
 		(set-process-query-on-exit-flag p nil)
 		(setq tramp-current-connection (cons vec (current-time)))
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index bb4ab9e3057..1aa4520eeb6 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -757,7 +757,7 @@ tramp-smb-action-get-acl
   "Read ACL data from connection buffer."
   (unless (process-live-p proc)
     ;; Accept pending output.
-    (while (tramp-accept-process-output proc))
+    (while (tramp-accept-process-output proc 0))
     (with-current-buffer (tramp-get-connection-buffer vec)
       ;; There might be a hidden password prompt.
       (widen)
@@ -1363,7 +1363,7 @@ tramp-smb-action-set-acl
   "Set ACL data."
   (unless (process-live-p proc)
     ;; Accept pending output.
-    (while (tramp-accept-process-output proc))
+    (while (tramp-accept-process-output proc 0))
     (tramp-message
      vec 10 "\n%s" (tramp-get-buffer-string (tramp-get-connection-buffer vec)))
     (throw 'tramp-action 'ok)))
@@ -2023,7 +2023,7 @@ tramp-smb-wait-for-output
 
       ;; Read pending output.
       (while (not (re-search-forward tramp-smb-prompt nil t))
-	(while (tramp-accept-process-output p))
+	(while (tramp-accept-process-output p 0))
 	(goto-char (point-min)))
       (tramp-message vec 6 "\n%s" (buffer-string))
 
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index 3cacde2468c..abb9afc570b 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -692,7 +692,7 @@ tramp-sudoedit-action-sudo
   "Check, whether a sudo process has finished.  Remove unneeded output."
   ;; There might be pending output for the exit status.
   (unless (process-live-p proc)
-    (while (tramp-accept-process-output proc))
+    (while (tramp-accept-process-output proc 0))
     ;; Delete narrowed region, it would be in the way reading a Lisp form.
     (goto-char (point-min))
     (widen)
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index b6e985db6b1..47173b95bea 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -5087,11 +5087,6 @@ tramp-handle-make-process
 	    ;; t.  See Bug#51177.
 	    (when filter
 	      (set-process-filter p filter))
-	    (process-put p 'vector v)
-	    ;; This is neded for ssh or PuTTY based processes, and
-	    ;; only if the respective options are set.  Perhaps, the
-	    ;; setting could be more fine-grained.
-	    (process-put p 'shared-socket t)
 	    (process-put p 'remote-command orig-command)
 	    (tramp-set-connection-property p "remote-command" orig-command)
 
@@ -5494,7 +5489,7 @@ tramp-handle-file-notify-rm-watch
   ;; There might be pending output.  Avoid problems with reentrant
   ;; call of Tramp.
   (ignore-errors
-    (while (tramp-accept-process-output proc)))
+    (while (tramp-accept-process-output proc 0)))
   (tramp-message proc 6 "Kill %S" proc)
   (delete-process proc))
 
@@ -5646,13 +5641,13 @@ tramp-action-process-alive
   "Check, whether a process has finished."
   (unless (process-live-p proc)
     ;; There might be pending output.
-    (while (tramp-accept-process-output proc))
+    (while (tramp-accept-process-output proc 0))
     (throw 'tramp-action 'process-died)))
 
 (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.
-  (while (tramp-accept-process-output proc))
+  (while (tramp-accept-process-output proc 0))
   (cond ((and (not (process-live-p proc))
 	      (zerop (process-exit-status proc)))
 	 (tramp-message	vec 3 "Process has finished.")
@@ -5683,7 +5678,7 @@ tramp-process-one-action
     (while (not found)
       ;; Reread output once all actions have been performed.
       ;; Obviously, the output was not complete.
-      (while (tramp-accept-process-output proc))
+      (while (tramp-accept-process-output proc 0))
       (setq todo actions)
       (while todo
 	(setq item (pop todo)
@@ -5800,21 +5795,11 @@ with-tramp-locked-connection
 	   ,@body)
        (tramp-flush-connection-property ,proc "locked"))))
 
-(defun tramp-accept-process-output (proc &optional _timeout)
+(defun tramp-accept-process-output (proc &optional timeout)
   "Like `accept-process-output' for Tramp processes.
 This is needed in order to hide `last-coding-system-used', which is set
 for process communication also.
 If the user quits via `C-g', it is propagated up to `tramp-file-name-handler'."
-  (declare (advertised-calling-convention (proc) "29.2"))
-  ;; There could be other processes which use the same socket for
-  ;; communication.  This could block the output for the current
-  ;; process.  Read such output first.  (Bug#61350)
-  (when-let (((process-get proc 'shared-socket))
-	     (v (process-get proc 'vector)))
-    (dolist (p (delq proc (process-list)))
-      (when (tramp-file-name-equal-p v (process-get p 'vector))
-	(accept-process-output p 0 nil t))))
-
   (with-current-buffer (process-buffer proc)
     (let ((inhibit-read-only t)
 	  last-coding-system-used
@@ -5824,10 +5809,10 @@ tramp-accept-process-output
 	;; JUST-THIS-ONE is set due to Bug#12145.  `with-local-quit'
 	;; returns t in order to report success.
 	(if (with-local-quit
-	      (setq result (accept-process-output proc 0 nil t)) t)
+	      (setq result (accept-process-output proc timeout nil t)) t)
 	    (tramp-message
-	     proc 10 "%s %s %s\n%s"
-	     proc (process-status proc) result (buffer-string))
+	     proc 10 "%s %s %s %s\n%s"
+	     proc timeout (process-status proc) result (buffer-string))
 	  ;; Propagate quit.
 	  (keyboard-quit)))
       result)))
@@ -6840,7 +6825,7 @@ tramp-interrupt-process
                  (tramp-get-remote-null-device (process-get proc 'vector))))
 	;; Wait, until the process has disappeared.  If it doesn't,
 	;; fall back to the default implementation.
-        (while (tramp-accept-process-output proc))
+        (while (tramp-accept-process-output proc 0))
 	(not (process-live-p proc))))))
 
 (add-hook 'interrupt-process-functions #'tramp-interrupt-process)
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-A-simpler-fix-for-bug-61350-a-small-tweak-Michael-s-.patch --]
[-- Type: text/x-patch, Size: 1161 bytes --]

From 08f32d4026bcd657c78d147e61af467d88e12748 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Wed, 15 Mar 2023 20:02:43 +0000
Subject: [PATCH 3/4] A simpler fix for bug#61350, a small tweak Michael's
 original idea.

* lisp/net/tramp.el (tramp-accept-process-output): Accept process
from related processes.
---
 lisp/net/tramp.el | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 47173b95bea..885b29f9471 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -5800,6 +5800,11 @@ tramp-accept-process-output
 This is needed in order to hide `last-coding-system-used', which is set
 for process communication also.
 If the user quits via `C-g', it is propagated up to `tramp-file-name-handler'."
+  (when-let (((process-get proc 'shared-socket))
+	     (v (process-get proc 'vector)))
+    (dolist (p (delq proc (process-list)))
+      (when (tramp-file-name-equal-p v (process-get p 'vector))
+	(while (accept-process-output p 0 nil t)))))
   (with-current-buffer (process-buffer proc)
     (let ((inhibit-read-only t)
 	  last-coding-system-used
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Just-for-testing-Remove-Tramp-specific-workaround-in.patch --]
[-- Type: text/x-patch, Size: 1935 bytes --]

From 7bb5dc7cfa7ff3d553369270f7cc6cd9868c69e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Wed, 15 Mar 2023 19:52:34 +0000
Subject: [PATCH 4/4] Just for testing: Remove Tramp-specific workaround in
 Eglot

* lisp/progmodes/eglot.el (eglot--connect): No longer bind tramp vars.
(tramp-ssh-controlmaster-options)
(tramp-use-ssh-controlmaster-options): Remove.
---
 lisp/progmodes/eglot.el | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6c1b9eafe43..f96f7dd254f 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -130,8 +130,6 @@
 (defvar markdown-fontify-code-blocks-natively)
 (defvar company-backends)
 (defvar company-tooltip-align-annotations)
-(defvar tramp-ssh-controlmaster-options)
-(defvar tramp-use-ssh-controlmaster-options)
 
 \f
 ;;; User tweakable stuff
@@ -1251,15 +1249,7 @@ eglot--connect
                         (contact (cl-subseq contact 0 probe)))
                    `(:process
                      ,(lambda ()
-                        (let ((default-directory default-directory)
-                              ;; bug#61350: Tramp turns on a feature
-                              ;; by default that can't (yet) handle
-                              ;; very much data so we turn it off
-                              ;; unconditionally -- just for our
-                              ;; process.
-                              (tramp-use-ssh-controlmaster-options t)
-                              (tramp-ssh-controlmaster-options
-                               "-o ControlMaster=no -o ControlPath=none"))
+                        (let ((default-directory default-directory))
                           (make-process
                            :name readable-name
                            :command (setq server-info (eglot--cmd contact))
-- 
2.39.2


  reply	other threads:[~2023-03-16 15:12 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-14 23:08 bug#62194: 30.0.50; Two Eglot-over-Tramp tests are failing on master, passing on emacs-29 João Távora
2023-03-15  9:40 ` Michael Albinus
2023-03-15 11:45   ` Michael Albinus
2023-03-15 20:24     ` João Távora
2023-03-15 20:36     ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-03-15 20:45       ` João Távora
2023-03-16 12:02         ` Michael Albinus
2023-03-16 12:20           ` João Távora
2023-03-16 14:57             ` Michael Albinus
2023-03-16 15:12               ` João Távora [this message]
2023-03-16 17:35                 ` Michael Albinus
2023-03-16 17:59                   ` João Távora
2023-03-16 21:18                     ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-03-16 21:57                       ` João Távora
2023-03-16 23:38                         ` João Távora
2023-03-17 16:45                         ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-03-17 16:47                           ` Eli Zaretskii
2023-03-17 17:22                             ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-03-17 17:22                               ` Eli Zaretskii
2023-03-17 10:44                     ` Michael Albinus
2023-03-17 11:19                       ` João Távora
2023-03-18  9:38                         ` Michael Albinus
2023-03-18 11:29                           ` João Távora
2023-03-18 12:23                             ` Michael Albinus
2023-03-18 12:33                               ` João Távora
2023-03-19 12:19                           ` Michael Albinus
2023-03-15 20:16   ` João Távora
2023-03-16 15:02     ` Michael Albinus
2023-03-28 10:51       ` Michael Albinus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87fsa43f36.fsf@gmail.com \
    --to=joaotavora@gmail.com \
    --cc=62194@debbugs.gnu.org \
    --cc=michael.albinus@gmx.de \
    --cc=miha@kamnitnik.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.