search

emacs-lsp / lsp-mode

Emacs client/library for the Language Server Protocol
https://emacs-lsp.github.io/lsp-mode
GNU General Public License v3.0
4.79k stars 890 forks source link

Remote mode not working due to CR/LF conversion #2375

Open martin5233 opened 3 years ago

martin5233 commented 3 years ago

I have set up clangd for lsp-mode, which works fine, when the clangd server is on the same machine as emacs. I have also attempted to use this remotely. In this case both involved machines are Linux, Emacs version is 27.1. The definition of the lsp client is as follows:

    (lsp-register-client
     (make-lsp-client :new-connection (lsp-tramp-connection "wrap-clangd.sh")
                      :major-modes '(c-mode c++-mode)
                      :priority -1
                      :remote? t
                      :server-id 'clangd-remote))))

I have written a small wrapper script as found somewhere on the internet, which redirects stdout and stderr of clangd to separate files. When trying to start the remote server, emacs endlessly displays 'clangd-remote: status: starting'. The stderr output of clangd contains the following information:

I[08:40:24.923] clangd version 11.0.0 (https://github.com/llvm/llvm-project.git 414f32a9e862b11f51063b75729278f8d81b12e9)
I[08:40:24.923] PID: 2531726
I[08:40:24.923] Working directory: /scratch/work
I[08:40:24.923] argv[0]: /usr/local/bin/clangd
I[08:40:24.923] Starting LSP over stdin/stdout
E[08:40:24.924] JSON parse error: [3:2081, byte=2083]: Expected , or } after object property

The stdout contains the expected initial contact string, which is rather long, so I won't paste it here. The interesting part is, that this string does not contain the CR characters after the initial Content-Length field, which are required by the LSP protocol. When starting clangd locally, these CR charaters are there. So I suspect, that TRAMP somehow swallows the CR characters, which causes clangd to complain about a wrong content length. I haven't found the place, where TRAMP converts line-endings, but I suspect, that the interaction between lsp-mode and TRAMP is somehow the problem.

yyoncho commented 3 years ago

Can you test by setting inhibit-eol-conversion to t? I think we had issues with this in the past.

martin5233 commented 3 years ago

Hi @yyoncho, thanks a lot for your quick response. Unfortunately changing this setting doesn't change anything. Is there anything else that I can try?

yyoncho commented 3 years ago

I researched that in the past but I didn't remember what was the solution if any. What we can try is to isolate an example and go and ask the tramp maintainer because I am not really familiar with these internal details. Also, the issue is env specific.

I would try also changing the following line in tramp connection (set-process-coding-system proc 'binary 'binary) to (set-process-coding-system result 'no-conversion 'no-conversion)

martin5233 commented 3 years ago

Hi @yyoncho: I tried changing the process coding system as you suggested, but still no improvement. I made an additional weird observation: When I kill clangd each time, when it hangs after producing the error, the connection will eventually succeed after 10-20 attempts. That means, that there is some strange phenomenon, which either depends on timing or other environment issues. After the connection succeeds, I can work with that same clangd instance forever. But when I attempt to start another clangd instance, the problem occurs again.

martin5233 commented 3 years ago

@yyoncho: I just looked at the code, where set-process-coding-system is called and there's one thing, I don't understand. If I understand the code correctly, then start-file-process-shell-command is invoked before setting the coding system, which seems wrong to me. Shouldn't all the process settings be done before invoking start-file-process-shell-command?

yyoncho commented 3 years ago

Shouldn't all the process settings be done before invoking start-file-process-shell-command?

IMHO it should be fine since we haven't started writing to that process and that conversion is only an emacs thing.

petr-tik commented 3 years ago

When I kill clangd each time, when it hangs after producing the error, the connection will eventually succeed after 10-20 attempts. That means, that there is some strange phenomenon, which either depends on timing or other environment issues. After the connection succeeds, I can work with that same clangd instance forever. But when I attempt to start another clangd instance, the problem occurs again.

I am experiencing the same problem with clangd over tramp as I previously mentioned on https://github.com/emacs-lsp/lsp-mode/issues/1845#issuecomment-722015244. I cannot share logs unfortunately, however I can confirm that the error message is the same "Expected , or } after object property" in lsp-log and when I examine the stdin of the clangd process on remote, I find seemingly correct JSON missing the last }.

I can also confirm that every once in a while LSP does start successfully and the } is indeed sent with the initailisation request. Haven't been able to reproduce deterministically and don't really know what variables/functions to inspect next time it happens.

realwhz commented 3 years ago

I recently started using lsp-mode. It works properly on my Linux box. However, yesterday I tried it on my macbook and got the same error. Specifically, my Linux box and MacBook remotely connect to the same Linux server and MacBook instance has the issue.

petr-tik commented 3 years ago

Specifically, my Linux box and MacBook remotely connect to the same Linux server and MacBook instance has the issue.

If I understand you correctly, you have 2 different clients - Mac and Linux connecting to a remote Linux box and only one of them connects successfully?

This could be a great way to reproduce. Can you please share more details about your emacs configuration and version on both clients as well as the lsp server version on the remote machine?

realwhz commented 3 years ago

@petr-tik yes, Mac and Linux laptops connect to the same remote Linux server and Mac has the issue. Here are the configurations:

Linux laptop (working): Arch Linux, GNU Emacs 27.1, lsp-mode 20201206.1844 Macbook (not working): GNU Emacs 27.1, lsp-mode 20201206.1844

Emacs configuration for lsp-mode (lsp-register-client (make-lsp-client :new-connection (lsp-tramp-connection "clangd") :major-modes '(c-mode c++-mode) :remote? t :server-id 'clangd-remote))

When error happened, the error message on remote Linux server says: I[17:01:49.923] clangd version 10.0.1 (Red Hat 10.0.1-1.module+el8.3.0+7459+90c24896) I[17:01:49.923] PID: 1972074 I[17:01:49.923] Working directory: /opt/git/cts I[17:01:49.923] argv[0]: clangd I[17:01:49.923] Starting LSP over stdin/stdout E[17:01:49.925] JSON parse error: [3:2113, byte=2115]: Expected , or } after object property E[17:35:27.832] Transport error: Input/output error I[17:35:27.833] LSP finished, exiting with status 1

yyoncho commented 3 years ago

I asked the tramp maintainer in emacs-devel, hopefully, soon we will come up with a solution.

petr-tik commented 3 years ago

I asked the tramp maintainer in emacs-devel, hopefully, soon we will come up with a solution.

https://lists.gnu.org/archive/html/emacs-devel/2020-12/msg00498.html

To clarify, I was echoing the original issue report

I have set up clangd for lsp-mode ... In this case both involved machines are Linux

Both machines are also linux in my case, so I am not sure if the mac-specific fix will be enough. Fingers crossed.

yyoncho commented 3 years ago

Can someone who can reproduce the issue jump into the emacs-devel thread? Michael is asking for that.

petr-tik commented 3 years ago

my problems aren't mac-related, but I also have issues with clangd over tramp and have reproduced it with pyls before

yyoncho commented 3 years ago

That could work too I guess. Let's hope it is the same issue.

realwhz commented 3 years ago

If Michael provides instructions and patches, I would like to try on my Mac. Just cannot guarantee response in real-time ...

martin5233 commented 3 years ago

@yyoncho: I have been in contact with Michael regarding this issue. He seems to have a patch for lsp-mode, which might fix this. I will try it out on my side and feed back here, if it works for me. See the thread on the emacs-devel mailing list for details.

petr-tik commented 3 years ago

Judging from this message, Michael fixed the issue in the tramp code and also changed his implementation of lsp-tramp-connection to this

(defun lsp-tramp-connection (local-command &optional generate-error-file-fn)
  "Create LSP stdio connection named name.
LOCAL-COMMAND is either list of strings, string or function which
returns the command to execute."
  ;; Force a direct asynchronous process.
  (add-to-list 'tramp-connection-properties
               (list (regexp-quote (file-remote-p default-directory))
                     "direct-async-process" t))
  (list :connect (lambda (filter sentinel name environment-fn)
                   (let* ((final-command (lsp-resolve-final-function 
local-command))
                          (_stderr (or (when generate-error-file-fn
                                        (funcall generate-error-file-fn name))
                                      (format "/tmp/%s-%s-stderr" name
                                              (cl-incf lsp--stderr-index))))
                          (process-name (generate-new-buffer-name name))
                          (process-environment
                           (lsp--compute-process-environment environment-fn))
                          (proc (make-process
                                 :name process-name
                                 :buffer (format "*%s*" process-name)
                                 :command final-command
                                 :connection-type 'pipe
                                 :coding 'no-conversion
                                 :noquery t
                                 :filter filter
                                 :sentinel sentinel
                                 :file-handler t)))
                     (cons proc proc)))
        :test? (lambda () (-> local-command lsp-resolve-final-function 
lsp-server-present?))))

Martin, can you please confirm if it works for you and does that mean that a) lsp-mode should require tramp version >2.5 b) lsp-mode needs to change its implementation of lsp-tramp-connection?

martin5233 commented 3 years ago

@petr-tik: I have built emacs from the master branch about 2 weeks ago, which includes the latest tramp changes. I also had to change the implementation of the connection setup in lsp-mode, so both your questions are a 'yes'. Unfortunately there is no released version of Emacs/Tramp yet, which contains the necessary changes. I don't know, if the new implementation of lsp-tramp-connection is usable without the other tramp changes.

petr-tik commented 3 years ago

Can you please confirm the SHA of tramp that fixes lsp-mode over tramp? I wanted to add something to docs/remote.md with troubleshooting instructions.

@yyoncho would you be open to changing the definition of lsp-tramp-connection to the one implemented by Michael Albinus?

apmanol commented 3 years ago

Thanks for reporting the issue. Same problem here with:

Client (fedora core 33):

GNU Emacs 27.1.90 (build 1, x86_64-redhat-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0) of 2020-12-19

     Status: Installed in ‘lsp-mode-20210105.1737/’ (unsigned).
    Version: 20210105.1737
     Commit: 5de768ff59e39d7014da96b86e8d8e4b2e6c6426

Package tramp is built-in.

     Status: Built-In.
    Version: 2.4.5.27.2

Server (fedora core 32):

clangd version 12.0.0 (https://github.com/llvm/llvm-project ac90bbc9cb8b905e4a8e7c9d2924a4d426c690aa)
cat  /tmp/c++-remote-2-stderr 
I[12:13:28.005] clangd version 12.0.0 (https://github.com/llvm/llvm-project ac90bbc9cb8b905e4a8e7c9d2924a4d426c690aa)
I[12:13:28.005] PID: 1798861
I[12:13:28.005] Working directory: /home/me/Projects/ms
I[12:13:28.005] argv[0]: clangd
I[12:13:28.005] Starting LSP over stdin/stdout
E[12:13:28.006] JSON parse error: [3:2144, byte=2146]: Expected , or } after object property
E[15:23:53.877] Transport error: Input/output error
I[15:23:53.877] LSP finished, exiting with status 1

The funny things is that sometime it works, without doing any thing :-)

How can we try the updated tramp version?

petr-tik commented 3 years ago

The funny things is that sometime it works, without doing any thing :-)

Same. I still haven't found a way to hunt the intermittent failure down.

How can we try the updated tramp version?

2 options:

  1. Compile an emacs binary from upstream source - this is bound to include the new version of tramp (that's what I did)
  2. git clone tramp from upstream and load it into your current emacs - Instructions put taken from here https://www.gnu.org/software/emacs/manual/html_node/tramp/Obtaining-TRAMP.html and here https://git.savannah.gnu.org/cgit/tramp.git/tree/INSTALL#n178
$ git clone git://git.savannah.gnu.org/tramp.git tramp_master
$ cd tramp_master/
$ autoconf

Add this to your config, I guess after tramp is loaded normally to override it.

     (add-to-list 'load-path "tramp_master")
     (require 'tramp)

If you try (2) - please report if that works, in which case @yyoncho we could add this clarification to the remote tutorial

apmanol commented 3 years ago

Unfortunately no safe conclusion can be made.

$git clone git://git.savannah.gnu.org/tramp.git
$cd tramp/
$ git lg -1 
* 4dd9606c - (HEAD -> master, origin/master, origin/HEAD) * tramp.texi (Quick Start Guide): Fix thinko. (2 days ago) <Michael Albinus>
$autoreconf -iv
$./configure 
$make all 
$make install DESTDIR=/tmp/mytramp

The .emacs:

(use-package tramp
  :defer t
  :load-path "/tmp/mytramp/usr/local/share/emacs/site-lisp/"
  :config
  ;; (setq tramp-verbose 6)

  ;; upstream value, takes into account recent ssh messages
  (setq tramp-yesno-prompt-regexp (concat
                                   (regexp-opt
                                    '("Are you sure you want to continue connecting (yes/no)?"
                                      "Are you sure you want to continue connecting (yes/no/[fingerprint])?")
                                    t)
                                   "\\s-*"))

  ;; disable vc for remote files (speed increase)
  (setq vc-ignore-dir-regexp
        (format "\\(%s\\)\\|\\(%s\\)"
                vc-ignore-dir-regexp
                tramp-file-name-regexp)))

but the problem still remains. (no change in the lsp-mode)

After commenting out the :load-path xxxx it started working with version Package: tramp (2.5.0 master/ac3829aa839cf70a8dd2e70c37eb59767c691a5f) in the next start of emacs.

martin5233 commented 3 years ago

@petr-tik: As Michael has fixed another issue, which arose in conjunction with magit and his Tramp changes, I have just pulled the latest version of Emacs and rebuilt. I can confirm, that remote lsp works with commit :9b31802e2d13cf8478c23d651741b54ffd4b984b.

c-nixon commented 3 years ago

The new tramp connection code above only works for built-in tramp transports due to it's use of the new direct async process method. For example it doesn't work with docker-tramp.

R0flcopt3r commented 2 years ago

What is the current situation?

I get this error on emacs v28.0.60 with tramp v2.5.2pre from macos 12.0.1 to a fedora 34 remote machine.

tshu-w commented 2 years ago

@R0flcopt3r with modified lsp-tramp-connection and tramp v.2.5.2 pre, everything works fine for macos 12.0.1 to a ubuntu 20

R0flcopt3r commented 2 years ago

@tshu-w the lsp-tramp-connection mentioned in this thread errors out with Wrong type argument: stringp, nil when calling like this:

  (lsp-register-client
   (make-lsp-client :new-connection (lsp-tramp-connection "clangd")
                    :major-modes '(c-mode c++-mode)
                    :remote? t
                    :server-id 'clangd-remote))

Using the lsp-tramp-connection from #10 , also posted by you, errors out with Invalid Content-Lenght...., error from clangd. So I assume it's close to working, and what remains is some working wrapper script that fixes that issue. However all attempts at that gives me an error that whatever I feed it doesn't exist in my PATH, regardless of calling with the absolute path, or having the script actually be in my PATH.

tshu-w commented 2 years ago

Using the lsp-tramp-connection from #10 , also posted by you, errors out with Invalid Content-Lenght...., error from clangd.

My current lsp-tramp-connection. Can you toggle lsp-log-io and paste you *lsp-log* *clangd-remote* and *cland-remote:error* to see if I can help.

However all attempts at that gives me an error that whatever I feed it doesn't exist in my PATH, regardless of calling with the absolute path, or having the script actually be in my PATH.

For this error, maybe you could tried (add-to-list 'tramp-remote-path 'tramp-own-remote-path). Can M-: (executable-find "clangd" t) find you scripts?

R0flcopt3r commented 2 years ago

It does seem like that specific lsp-tramp-connection fixes it. I had some issues, but not super obvious how to replicate it, seems stable anyways.

The last remaining problem is how insanely slow it is in a 1.5 mil LOC C++ project... Takes literal minutes to find definitions.. It seems to only spawn 5 threads on the remote machine, while lsp-clients-clangd-args is set to '("-j7" ...). I don't think that is why it is slow. Probably has to do with how tramp does things. Had similar performance problems with find-file, but that was "trivial" to rewrite.

At least it seems to be nice and stable for smaller projects. Like my advent of code project, it's nice and snappy, about as running it locally.

Thanks a lot for your time, @tshu-w.

yyoncho commented 2 years ago

@R0flcopt3r

The last remaining problem is how insanely slow it is in a 1.5 mil LOC C++ project...

That can be optimized on lsp-mode side by not loading the line content. We don't have other option but to do that synchronously. Eventually, if xref does not support that we may create an alternative solution that resolves the line content lazily

R0flcopt3r commented 2 years ago

slight update on the performance issues. it doesn't seem like it uses the lsp functionality at all when searching for this specific symbol I was on. Below are first a report for the remote project, and next is from the same project but locally.

128  40%   - +lookup/definition
128  40%    - cond
128  40%     - +lookup--jump-to
128  40%      - let*
127  40%       - if
127  40%        - run-hook-wrapped
127  40%         - +lookup--run-handlers
127  40%          - condition-case
127  40%           - let
127  40%            - condition-case
127  40%             - +lookup--run-handler
127  40%              - if
 99  31%               - funcall
 91  29%                - +lookup-dumb-jump-backend-fn
 91  29%                 - and
 91  29%                  - dumb-jump-go
 69  22%                   - dumb-jump-get-results
 67  21%                    - dumb-jump-fetch-file-results
 54  17%                     - dumb-jump-get-project-root
 54  17%                      - locate-dominating-file
 50  15%                       - dumb-jump-get-config
 50  15%                        - tramp-file-name-handler
 42  13%                         - apply
 41  13%                          - tramp-sh-file-name-handler
 36  11%                           - tramp-sh-handle-file-exists-p
 31   9%                            - tramp-send-command-and-check
 31   9%                             - tramp-send-command
 21   6%                              - tramp-wait-for-output
 21   6%                               - tramp-wait-for-regexp
 21   6%                                - tramp-accept-process-output
  2   0%                                 + accept-process-output
  8   2%                                tramp-send-string
  2   0%                              tramp-get-file-property
  2   0%                            + tramp-set-file-property
  4   1%                           + tramp-sh-handle-expand-file-name
  1   0%                            tramp-file-name-for-operation
  6   1%                         + tramp-find-foreign-file-name-handler
  1   0%                           tramp-dissect-file-name
  1   0%                       + abbreviate-file-name
  1   0%                       + tramp-file-name-handler
  8   2%                     + dumb-jump-fetch-results
  5   1%                     + dumb-jump-get-config
 22   7%                   - dumb-jump-result-follow
 22   7%                    - dumb-jump--result-follow
 22   7%                     - dumb-jump-goto-file-line
 18   5%                      - find-file
 17   5%                       + find-file-noselect
  1   0%                       + pop-to-buffer-same-window
  4   1%                      + find-buffer-visiting
  8   2%                + +lookup-xref-definitions-backend-fn
 28   8%               + call-interactively
  1   0%       + unwind-protect
108  34%   + counsel-M-x
  1   0%  + list
 49  15% + ...
 19   6% + timer-event-handler
  8   2% + redisplay_internal (C function)

vs this locally:

161  41% - ...
142  36%    Automatic GC
 12   3%  - +lookup/definition
 12   3%   - cond
 12   3%    - +lookup--jump-to
 12   3%     - let*
 12   3%      - if
 12   3%       - run-hook-wrapped
 12   3%        - +lookup--run-handlers
 12   3%         - condition-case
 12   3%          - let
 12   3%           - condition-case
 12   3%            - +lookup--run-handler
 12   3%             - if
 12   3%              - call-interactively
 12   3%               - funcall-interactively
 12   3%                - lsp-ui-peek-find-definitions
 10   2%                 - lsp-ui-peek--find-xrefs
  6   1%                  - lsp-ui-peek--goto-xref
  3   0%                   + apply
  1   0%                   + switch-to-buffer
  1   0%                     lsp-ui-peek-mode
  2   0%                  + lsp-ui-peek--get-references
  2   0%                   symbol-at-point
  5   1%  - minibuffer-inactive-mode
  5   1%   + run-mode-hooks
  1   0%  + +doom-dashboard--get-pwd
  1   0%  + +ivy--set-jump-point-maybe-h
 22   5% + timer-event-handler
 11   2% + redisplay_internal (C function)
  2   0% + #<compiled 0x11e633e5f9f83c18>
  1   0% + evil-repeat-post-hook
tshu-w commented 2 years ago

It does seem like that specific lsp-tramp-connection fixes it. I had some issues, but not super obvious how to replicate it, seems stable anyways.

Hi, I add direct asynchronous process back to lsp-tramp-connection and I don't really know whether this work in this context, you can refer to this comment

albusshin commented 2 years ago

FWIW, with the lsp-tramp-connection implementation above doesn't work for me either, but @antifuchs's implementation with SSH port forwarding here: https://github.com/emacs-lsp/lsp-mode/issues/2709#issuecomment-860753973 worked for me.

highfly22 commented 1 year ago

From lsp communication log from server side, I found the messages were truncated to 4096 bytes. After searching this number, I got this. I fix the stty issue in the remote tramp by add '-icanon'.

It works with this comment under my linux box and emacs 28.2 tramp 2.5.3

--- tramp-sh.el.orig    2022-05-12 19:59:16.000000000 +0800
+++ tramp-sh.el 2023-01-28 11:14:40.371519390 +0800
@@ -2959,7 +2959,7 @@
              ;; macOS, see Bug#50748.
              (when (and (memq connection-type '(nil pipe))
                                      (not (tramp-check-remote-uname v "Darwin")))
-               (tramp-send-command v "stty -icrnl"))
+               (tramp-send-command v "stty -icrnl -icanon"))
              ;; `tramp-maybe-open-connection' and
              ;; `tramp-send-command-and-read' could have
              ;; trashed the connection buffer.  Remove this.
albinus commented 1 year ago

From lsp communication log from server side, I found the messages were truncated to 4096 bytes. After searching this number, I got this. I fix the stty issue in the remote tramp by add '-icanon'.

It works with this comment under my linux box and emacs 28.2 tramp 2.5.3

--- tramp-sh.el.orig  2022-05-12 19:59:16.000000000 +0800
+++ tramp-sh.el   2023-01-28 11:14:40.371519390 +0800
@@ -2959,7 +2959,7 @@
            ;; macOS, see Bug#50748.
            (when (and (memq connection-type '(nil pipe))
                                      (not (tramp-check-remote-uname v "Darwin")))
-             (tramp-send-command v "stty -icrnl"))
+             (tramp-send-command v "stty -icrnl -icanon"))
            ;; `tramp-maybe-open-connection' and
            ;; `tramp-send-command-and-read' could have
            ;; trashed the connection buffer.  Remove this.

Thanks for this comment.

Since this will be a serious change to Tramp, which could affect asynchronous proresses at all, I'd appreciate if somebody could write an Emacs bug report via 'M-x report-emacs-bug'. We'll discuss the issue there.

Note, that I won't discuss on github.

abbaswasim commented 1 year ago

What is the current situation on this bug? I have reported it on https://github.com/emacs-lsp/lsp-mode/issues/843 but this issue seems to have more recent updates and is open.

I am getting these issues on emacs GNU Emacs 28.2 and lsp version 20230801.1638 with tramp 2.6.1.1 on MacOs 13.5 to a arch linux remote machine running clangd 15.0.7.

Is it safe to assume lsp-mode changes to lsp-tramp-connection are upstream now? Anyways I have tried most of the updated version of lsp-tramp-connection in this and other threads including the one in doom emacs one linked somewhere in there but no luck.