bug#47861: Starting `jsonrpc-process-connection' over Tramp fails if the process writes to stderr

bug#47861: Starting `jsonrpc-process-connection' over Tramp fails if the process writes to stderr

[Jim Porter]

When starting a `jsonrpc-process-connection' over Tramp, if the
process writes to stderr, Emacs (usually) fails with `Wrong type
argument: "inserted-chars 204"'. I'm able to reproduce this pretty
consistently when connecting from an MS Windows client to a Linux
server; however, this may apply to other platform combinations as
well. Mysteriously, it works fine about 1 in 10 times, though I
haven't been able to decipher why.

This is a reduced test case from an eglot issue that prevents users
from using eglot with clangd over Tramp (or likely any LSP server that
writes to stderr): <https://github.com/joaotavora/eglot/issues/662>.
Michael Albinus has a workaround for this for the next version of
Tramp (2.5.0.4, I believe), but suggested I file an issue here to help
find a more-permanent solution.

The following recipe reproduces the issue for me on both Emacs 27.2
and the prebuilt Emacs 28 snapshot from 2021-01-15. `jsonrpc-minimal'
is a cut-down version of `eglot--connect'. Note that this test tries
to run `clangd', but any executable that writes to stderr on startup
should reproduce this issue (including a nonexistent executable).
After running "emacs -Q":

----------[ Eval the following ]----------
;; Optional: disable ControlMaster if using ssh on Win32; it doesn't work there.
(require 'tramp)
(setq tramp-use-ssh-controlmaster-options nil)

(require 'jsonrpc)
(defun jsonrpc-minimal ()
  (interactive)
  (make-instance
   'jsonrpc-process-connection
   :name "jsonrpc-minimal"
   :process (lambda ()
              (make-process
               :name "jsonrpc-minimal"
               :command '("sh" "-c" "stty raw > /dev/null; clangd")
               :connection-type 'pipe
               :coding 'utf-8-emacs-unix
               :noquery t
               :stderr (get-buffer-create "*jsonrpc-minimal stderr*")
               :file-handler t))))
----------[ End ]----------

Then run `C-x C-f /sshx:server:~/path/to/file.txt RET' followed by
`M-x jsonrpc-minimal'. (Other Tramp methods such as plink should also
reproduce this issue.) If `debug-on-error' is set, I get the following
backtrace:

----------[ Begin backtrace ]----------
  signal(wrong-type-argument ("inserted-chars 204"))
  tramp-signal-hook-function(wrong-type-argument ("inserted-chars 204"))
  signal(wrong-type-argument ("inserted-chars 204"))
  tramp-handle-insert-file-contents("/sshx:server:/tmp/tramp.lLZT4H"
nil nil nil nil)
  apply(tramp-handle-insert-file-contents
("/sshx:server:/tmp/tramp.lLZT4H" nil nil nil nil))
  tramp-sh-file-name-handler(insert-file-contents
"/sshx:server:/tmp/tramp.lLZT4H" nil nil nil nil)
  apply(tramp-sh-file-name-handler insert-file-contents
("/sshx:server:/tmp/tramp.lLZT4H" nil nil nil nil))
  tramp-file-name-handler(insert-file-contents
"/sshx:server:/tmp/tramp.lLZT4H" nil nil nil nil)
  insert-file-contents("/sshx:server:/tmp/tramp.lLZT4H" nil nil nil nil)
  insert-file-contents-literally("/sshx:server:/tmp/tramp.lLZT4H")
  tramp-sh-handle-make-process(:name "jsonrpc-minimal" :command ("sh"
"-c" "stty raw > /dev/null; clangd") :connection-type pipe :coding
utf-8-emacs-unix :noquery t :stderr #<buffer *jsonrpc-minimal stderr*>
:file-handler t)
  apply(tramp-sh-handle-make-process (:name "jsonrpc-minimal" :command
("sh" "-c" "stty raw > /dev/null; clangd") :connection-type pipe
:coding utf-8-emacs-unix :noquery t :stderr #<buffer *jsonrpc-minimal
stderr*> :file-handler t))
  tramp-sh-file-name-handler(make-process :name "jsonrpc-minimal"
:command ("sh" "-c" "stty raw > /dev/null; clangd") :connection-type
pipe :coding utf-8-emacs-unix :noquery t :stderr #<buffer
*jsonrpc-minimal stderr*> :file-handler t)
  apply(tramp-sh-file-name-handler make-process (:name
"jsonrpc-minimal" :command ("sh" "-c" "stty raw > /dev/null; clangd")
:connection-type pipe :coding utf-8-emacs-unix :noquery t :stderr
#<buffer *jsonrpc-minimal stderr*> :file-handler t))
  tramp-file-name-handler(make-process :name "jsonrpc-minimal"
:command ("sh" "-c" "stty raw > /dev/null; clangd") :connection-type
pipe :coding utf-8-emacs-unix :noquery t :stderr #<buffer
*jsonrpc-minimal stderr*> :file-handler t)
  make-process(:name "jsonrpc-minimal" :command ("sh" "-c" "stty raw >
/dev/null; clangd") :connection-type pipe :coding utf-8-emacs-unix
:noquery t :stderr #<buffer *jsonrpc-minimal stderr*> :file-handler t)
  (closure (t) nil (make-process :name "jsonrpc-minimal" :command
'("sh" "-c" "stty raw > /dev/null; clangd") :connection-type 'pipe
:coding 'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t))()
  #f(compiled-function (cl--cnm conn slots) #<bytecode
0x18017ea42140ed74>)(#f(compiled-function (&rest cnm-args) #<bytecode
0x69ccf178fb9f2f9>) #<jsonrpc-process-connection
jsonrpc-process-connection-192d52c> (:name "jsonrpc-minimal" :process
(closure (t) nil (make-process :name "jsonrpc-minimal" :command '("sh"
"-c" "stty raw > /dev/null; clangd") :connection-type 'pipe :coding
'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t))))
  apply(#f(compiled-function (cl--cnm conn slots) #<bytecode
0x18017ea42140ed74>) #f(compiled-function (&rest cnm-args) #<bytecode
0x69ccf178fb9f2f9>) (#<jsonrpc-process-connection
jsonrpc-process-connection-192d52c> (:name "jsonrpc-minimal" :process
(closure (t) nil (make-process :name "jsonrpc-minimal" :command '("sh"
"-c" "stty raw > /dev/null; clangd") :connection-type 'pipe :coding
'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t)))))
  #f(compiled-function (&rest args) #<bytecode
0x101ee737e4a4203a>)(#<jsonrpc-process-connection
jsonrpc-process-connection-192d52c> (:name "jsonrpc-minimal" :process
(closure (t) nil (make-process :name "jsonrpc-minimal" :command '("sh"
"-c" "stty raw > /dev/null; clangd") :connection-type 'pipe :coding
'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t))))
  apply(#f(compiled-function (&rest args) #<bytecode
0x101ee737e4a4203a>) #<jsonrpc-process-connection
jsonrpc-process-connection-192d52c> (:name "jsonrpc-minimal" :process
(closure (t) nil (make-process :name "jsonrpc-minimal" :command '("sh"
"-c" "stty raw > /dev/null; clangd") :connection-type 'pipe :coding
'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t))))
  initialize-instance(#<jsonrpc-process-connection
jsonrpc-process-connection-192d52c> (:name "jsonrpc-minimal" :process
(closure (t) nil (make-process :name "jsonrpc-minimal" :command '("sh"
"-c" "stty raw > /dev/null; clangd") :connection-type 'pipe :coding
'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t))))
  #f(compiled-function (class &rest slots) "Default constructor for
CLASS `eieio-default-superclass'.\nSLOTS are the initialization slots
used by `initialize-instance'.\nThis static method is called when an
object is constructed.\nIt allocates the vector used to represent an
EIEIO object, and then\ncalls `initialize-instance' on that object."
#<bytecode -0x1cebd9ffedfcea53>)(jsonrpc-process-connection :name
"jsonrpc-minimal" :process (closure (t) nil (make-process :name
"jsonrpc-minimal" :command '("sh" "-c" "stty raw > /dev/null; clangd")
:connection-type 'pipe :coding 'utf-8-emacs-unix :noquery t :stderr
(get-buffer-create "*jsonrpc-minimal stderr*") :file-handler t)))
  apply(#f(compiled-function (class &rest slots) "Default constructor
for CLASS `eieio-default-superclass'.\nSLOTS are the initialization
slots used by `initialize-instance'.\nThis static method is called
when an object is constructed.\nIt allocates the vector used to
represent an EIEIO object, and then\ncalls `initialize-instance' on
that object." #<bytecode -0x1cebd9ffedfcea53>)
jsonrpc-process-connection (:name "jsonrpc-minimal" :process (closure
(t) nil (make-process :name "jsonrpc-minimal" :command '("sh" "-c"
"stty raw > /dev/null; clangd") :connection-type 'pipe :coding
'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t))))
  make-instance(jsonrpc-process-connection :name "jsonrpc-minimal"
:process (closure (t) nil (make-process :name "jsonrpc-minimal"
:command '("sh" "-c" "stty raw > /dev/null; clangd") :connection-type
'pipe :coding 'utf-8-emacs-unix :noquery t :stderr (get-buffer-create
"*jsonrpc-minimal stderr*") :file-handler t)))
  jsonrpc-minimal()
  funcall-interactively(jsonrpc-minimal)
  call-interactively(jsonrpc-minimal record nil)
  command-execute(jsonrpc-minimal record)
  execute-extended-command(nil "jsonrpc-minimal" "jsonrpc-minimal")
  funcall-interactively(execute-extended-command nil "jsonrpc-minimal"
"jsonrpc-minimal")
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)
----------[ End backtrace ]----------

----------[ Begin Emacs debug info ]----------
In GNU Emacs 28.0.50 (build 1, x86_64-w64-mingw32)
 of 2021-01-15 built on CIRROCUMULUS
Repository revision: f45be48ddbde00610e1e08fca6590dcf24a4e1b5
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 10.0.19041
System Description: Microsoft Windows 10 Pro (v10.0.2004.19041.928)

Configured using:
 'configure --without-dbus --without-compress-install -C 'CFLAGS=-O2
 -static -g3' PKG_CONFIG_PATH=/mingw64/lib/pkgconfig'

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
W32NOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS XPM
ZLIB

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1252

Major mode: Fundamental

Minor modes in effect:
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils mule-util help-fns
radix-tree cl-print cus-start cus-load vc-hg vc-git diff-mode easy-mmode
vc-bzr cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align
cc-engine cc-vars cc-defs tramp-cache tramp-sh jsonrpc ert pp ewoc debug
backtrace help-mode easymenu find-func pcase warnings tramp
tramp-loaddefs trampver tramp-integration files-x tramp-compat shell
pcomplete comint ansi-color ring parse-time iso8601 format-spec
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map seq byte-opt gv bytecomp byte-compile cconv
time-date subr-x cl-loaddefs cl-lib iso-transl tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel dos-w32 ls-lisp
disp-table term/w32-win w32-win w32-vars term/common-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads w32notify
w32 lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 148954 7276)
 (symbols 48 13698 1)
 (strings 32 40410 1316)
 (string-bytes 1 1461011)
 (vectors 16 21458)
 (vector-slots 8 300360 13838)
 (floats 8 55 160)
 (intervals 56 2822 0)
 (buffers 984 18))
----------[ End Emacs debug info ]----------

bug#47861: Starting `jsonrpc-process-connection' over Tramp fails if the process writes to stderr

[Michael Albinus]

Jim Porter <jporterbugs@gmail.com> writes:

> When starting a `jsonrpc-process-connection' over Tramp, if the
> process writes to stderr, Emacs (usually) fails with `Wrong type
> argument: "inserted-chars 204"'. I'm able to reproduce this pretty
> consistently when connecting from an MS Windows client to a Linux
> server; however, this may apply to other platform combinations as
> well. Mysteriously, it works fine about 1 in 10 times, though I
> haven't been able to decipher why.

In current Tramp 2.5.0.4, this has been mitigated by wrapping the
insert-file-contents-literally call in tramp-sh-handle-make-process with
ignore-errors.

However, handling stderr buffers in Tramp's make-process is still a
mess. So I've started to reimplement this, using a named pipe on the
remote machine. make-pipe-process, the natural choice, does not work for
remote processes.

Best regards, Michael.

bug#47861: Starting `jsonrpc-process-connection' over Tramp fails if the process writes to stderr

[Michael Albinus]

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

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

Hi Jim,

> However, handling stderr buffers in Tramp's make-process is still a
> mess. So I've started to reimplement this, using a named pipe on the
> remote machine. make-pipe-process, the natural choice, does not work for
> remote processes.

I've finished a first shot on this, see the appended patch. Would you
mind to test this in your environment, for example with eglot?

Thanks, and best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 6099 bytes --]

diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el
index 60090d31..22d31f82 100644
--- a/lisp/tramp-sh.el
+++ b/lisp/tramp-sh.el
@@ -2723,13 +2723,12 @@ the result will be a local, non-Tramp, file name."
 ;; We use BUFFER also as connection buffer during setup. Because of
 ;; this, its original contents must be saved, and restored once
 ;; connection has been setup.
-;; The complete STDERR buffer is available only when the process has
-;; terminated.
 (defun tramp-sh-handle-make-process (&rest args)
   "Like `make-process' for Tramp files.
-STDERR can also be a file name.  If method parameter `tramp-direct-async'
-and connection property \"direct-async-process\" are non-nil, an
-alternative implementation will be used."
+STDERR can also be a remote file name.  If method parameter
+`tramp-direct-async' and connection property
+\"direct-async-process\" are non-nil, an alternative
+implementation will be used."
   (if (tramp-direct-async-process-p args)
       (apply #'tramp-handle-make-process args)
     (when args
@@ -2763,7 +2762,7 @@ alternative implementation will be used."
 	    (signal 'wrong-type-argument (list #'functionp sentinel)))
 	  (unless (or (null stderr) (bufferp stderr) (stringp stderr))
 	    (signal 'wrong-type-argument (list #'bufferp stderr)))
-	  (when (and (stringp stderr) (tramp-tramp-file-p stderr)
+	  (when (and (stringp stderr)
 		     (not (tramp-equal-remote default-directory stderr)))
 	    (signal 'file-error (list "Wrong stderr" stderr)))

@@ -2775,9 +2774,9 @@ alternative implementation will be used."
 		 ;; STDERR can also be a file name.
 		 (tmpstderr
 		  (and stderr
-		       (if (and (stringp stderr) (tramp-tramp-file-p stderr))
-			   (tramp-unquote-file-local-name stderr)
-			 (tramp-make-tramp-temp-file v))))
+		       (tramp-unquote-file-local-name
+			(if (stringp stderr)
+			    stderr (tramp-make-tramp-temp-name v)))))
 		 (remote-tmpstderr
 		  (and tmpstderr (tramp-make-tramp-file-name v tmpstderr)))
 		 (program (car command))
@@ -2786,7 +2785,8 @@ alternative implementation will be used."
 		 ;; "-c", it might be that the arguments exceed the
 		 ;; command line length.  Therefore, we modify the
 		 ;; command.
-		 (heredoc (and (stringp program)
+		 (heredoc (and (not (bufferp stderr))
+			       (stringp program)
 			       (string-match-p "sh$" program)
 			       (= (length args) 2)
 			       (string-equal "-c" (car args))
@@ -2850,6 +2850,23 @@ alternative implementation will be used."
 		 tramp-current-connection
 		 p)

+	    ;; Handle error buffer.
+	    (when (bufferp stderr)
+	      (with-current-buffer stderr
+		(setq buffer-read-only nil))
+	      ;; Create named pipe.
+	      (tramp-send-command v (format "mknod %s p" tmpstderr))
+	      ;; Create stderr process.
+	      (make-process
+	       :name (buffer-name stderr)
+	       :buffer stderr
+	       :command `("cat" ,tmpstderr)
+	       :coding coding
+	       :noquery t
+	       :filter nil
+	       :sentinel #'ignore
+	       :file-handler t))
+
 	    (while (get-process name1)
 	      ;; NAME must be unique as process name.
 	      (setq i (1+ i)
@@ -2912,38 +2929,16 @@ alternative implementation will be used."
 			(ignore-errors
 			  (set-process-query-on-exit-flag p (null noquery))
 			  (set-marker (process-mark p) (point)))
-			;; We must flush them here already; otherwise
-			;; `rename-file', `delete-file' or
-			;; `insert-file-contents' will fail.
-			(tramp-flush-connection-property v "process-name")
-			(tramp-flush-connection-property v "process-buffer")
-			;; Copy tmpstderr file.
-			(when (and (stringp stderr)
-				   (not (tramp-tramp-file-p stderr)))
-			  (add-function
-			   :after (process-sentinel p)
-			   (lambda (_proc _msg)
-			     (rename-file remote-tmpstderr stderr))))
-			;; Provide error buffer.  This shows only
-			;; initial error messages; messages arriving
-			;; later on will be inserted when the process
-			;; is deleted.  The temporary file will exist
-			;; until the process is deleted.
+			;; Kill stderr process and named pipe.
 			(when (bufferp stderr)
-			  (with-current-buffer stderr
-			    ;; There's a mysterious error, see
-			    ;; <https://github.com/joaotavora/eglot/issues/662>.
-			    (ignore-errors
-			      (insert-file-contents-literally remote-tmpstderr)))
-			  ;; Delete tmpstderr file.
 			  (add-function
 			   :after (process-sentinel p)
 			   (lambda (_proc _msg)
-			     (when (file-exists-p remote-tmpstderr)
-			       (with-current-buffer stderr
-				 (ignore-errors
-				   (insert-file-contents-literally
-				    remote-tmpstderr nil nil nil 'replace)))
+			     (ignore-errors
+			       (while (accept-process-output
+				       (get-buffer-process stderr) 0 nil t))
+			       (delete-process (get-buffer-process stderr)))
+			     (ignore-errors
 			       (delete-file remote-tmpstderr)))))
 			;; Return process.
 			p)))
@@ -4834,10 +4829,12 @@ connection if a previous connection has died for some reason."
 	  (with-tramp-progress-reporter
 	      vec 3
 	      (if (zerop (length (tramp-file-name-user vec)))
-		  (format "Opening connection for %s using %s"
+		  (format "Opening connection %s for %s using %s"
+			  process-name
 			  (tramp-file-name-host vec)
 			  (tramp-file-name-method vec))
-		(format "Opening connection for %s@%s using %s"
+		(format "Opening connection %s for %s@%s using %s"
+			process-name
 			(tramp-file-name-user vec)
 			(tramp-file-name-host vec)
 			(tramp-file-name-method vec)))
@@ -5937,8 +5934,6 @@ function cell is returned to be applied on a buffer."
 ;;   session could be reused after a connection loss.  Use dtach, or
 ;;   screen, or tmux, or mosh.
 ;;
-;; * Implement `:stderr' of `make-process' as pipe process.
-
 ;; * One interesting solution (with other applications as well) would
 ;;   be to stipulate, as a directory or connection-local variable, an
 ;;   additional rc file on the remote machine that is sourced every

bug#47861: Starting `jsonrpc-process-connection' over Tramp fails if the process writes to stderr

[Jim Porter]

On Sat, May 15, 2021 at 10:33 AM Michael Albinus <michael.albinus@gmx.de> wrote:
> Michael Albinus <michael.albinus@gmx.de> writes:
>
> > However, handling stderr buffers in Tramp's make-process is still a
> > mess. So I've started to reimplement this, using a named pipe on the
> > remote machine. make-pipe-process, the natural choice, does not work for
> > remote processes.
>
> I've finished a first shot on this, see the appended patch. Would you
> mind to test this in your environment, for example with eglot?

I've tested this patch (using commit
e89a2304a152d6ee9e5d75efe0d22cabab771388 from the Tramp repository,
which includes your above patch), and everything works correctly as
far as I can tell.

I also wanted to be sure I could still reproduce the original issue,
so I confirmed that if I roll Tramp back to a previous revision before
your workaround (I used commit
f9301e5d4424bd567dc212f579350c5e04397ba1), I see the bug again.

Thanks for the fix!

bug#47861: Starting `jsonrpc-process-connection' over Tramp fails if the process writes to stderr

[Michael Albinus]

Version: 28.1

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

>> I've finished a first shot on this, see the appended patch. Would you
>> mind to test this in your environment, for example with eglot?
>
> I've tested this patch (using commit
> e89a2304a152d6ee9e5d75efe0d22cabab771388 from the Tramp repository,
> which includes your above patch), and everything works correctly as
> far as I can tell.
>
> I also wanted to be sure I could still reproduce the original issue,
> so I confirmed that if I roll Tramp back to a previous revision before
> your workaround (I used commit
> f9301e5d4424bd567dc212f579350c5e04397ba1), I see the bug again.

Thanks for the feedback! I'm closing the bug, the patch will be included
in the next Tramp 2.5.0.5 version.

> Thanks for the fix!

Best regards, Michael.