lem icon indicating copy to clipboard operation
lem copied to clipboard

Process stdio mishandled in `lsp-mode`'s stdio client

Open resttime opened this issue 1 year ago • 8 comments

In the process of trying to add LSP support for c-mode through clangd a new issue appears. This is found to affect both SDL2 and ncurses frontends so it seems specific to backend.

First clangd only supports stdio but that's okay since there's an stdio client implemented for lsp-mode and the spec can be defined like this(?)

(define-language-spec (c-spec lem-c-mode:c-mode)
  :language-id "c"
  :root-uri-patterns '("Makefile")
  :command (lambda () '("clangd"))
  :readme-url "https://clangd.llvm.org/"
  :connection-mode :stdio)

However lsp-mode is stuck "initializing..." along with a modeline spinner spinning forever. Looking at the configuration for the go-mode language server gopls seems to work through TCP although so I tried to see if it'd be the same for clangd. I used socat to fork the clangd process and redirect stdio to a TCP port. lsp-mode can connect to a TCP server now that redirects to clangd process. The hack worked surprisingly and lsp-mode started working in the buffer.

(define-language-spec (c-spec lem-c-mode:c-mode)
  :language-id "c"
  :root-uri-patterns '("Makefile")
  :command (lambda (port) `("socat" ,(format nil "TCP-LISTEN:~a,fork" port) "exec:clangd"))
  :readme-url "https://clangd.llvm.org/"
  :connection-mode :tcp)

There must be a bug in how the stdio of processes are being handled, the JSONRPC, or etc. This continuation that gets passed does not get complete evaluation when the stdio client is used. If it did the spinner would stop spinning and initialization would finish: https://github.com/lem-project/lem/blob/d7ece6acc7a5cefc69f34c175d540680010fd01c/extensions/lsp-mode/lsp-mode.lisp#L393-L403

stdio client

image

tcp client

image

resttime avatar Sep 09 '23 06:09 resttime

Interesting! I did add a lsp config for elixir that uses stdio and it worked fined:

(define-language-spec (elixir-spec lem-elixir-mode:elixir-mode)
  :language-id "elixir"
  :root-uri-patterns '("mix.exs")
  :command '("sh" "language_server.sh")
  :install-command ""
  :readme-url "https://github.com/elixir-lsp/elixir-ls"
  :connection-mode :stdio)

I'll like to finish the https://github.com/lem-project/lem/pull/974 issue, so we can gather more information about the process behind (and potencially get some output of the incoming calls)

Sasanidas avatar Sep 09 '23 11:09 Sasanidas

Found a root cause which prob explains why the stdio client for elixir works. If clangd detects it's run in a "terminal" it outputs a message that lem tries parse as a header. Which throws an error because it does not follow the LSP protocol specification. The default behaviour on an error is to return nil.

https://github.com/lem-project/lem/blob/9ee5cef414c8fb89b807127353dfa7b5d4ef2baa/extensions/lsp-mode/lem-stdio-transport.lisp#L47-L50

This causes the thread which is running the loop processing the output from clangd to terminate. Thus nothing read gets returned an initialization cannot proceed.

https://github.com/lem-project/lem/blob/9ee5cef414c8fb89b807127353dfa7b5d4ef2baa/extensions/lsp-mode/lem-stdio-transport.lisp#L27-L31

Ultimately lem is opening the clangd process but not redirecting the process stdio as intended by clangd.

https://github.com/llvm/llvm-project/blob/c8387a31a4adfa9c29a578cf67321f756d3b4ac1/clang-tools-extra/clangd/tool/ClangdMain.cpp#L836

There are quick hacks that can been added to ignore/trim/throw away the message as a a solution, although it'd be specific to clangd only. The "correct" solution would be to understand how clangd checks it runs in terminal and modify how the process is being opened since that'd affect other processes similarly.

image

resttime avatar Sep 12 '23 07:09 resttime

Does clangd think it's in a terminal for the same reason as outlined here?

seanfarley avatar Sep 12 '23 17:09 seanfarley

That may be the case as PTY is mentioned a lot in the library lem uses.

https://github.com/lem-project/async-process/blob/9690530fc92b59636d9f17d821afa7697e7c8ca4/src/async-process.c#L38

resttime avatar Sep 12 '23 17:09 resttime

Much like that article mentions, we should be defaulting to PTY-less so as to avoid these kind of difficult-to-debug issues!

seanfarley avatar Sep 12 '23 22:09 seanfarley

The "correct" solution would be to understand how clangd checks it runs in terminal and modify how the process is being opened since that'd affect other processes similarly.

Just randomly popped here when checking out LEM. Are you sure clangd isn't really writing that message to stderr? Mine is. I think there's nothing in the spec against that. clangd writes lots of stuff to stderr, which one can use to make a (very useful, IMHO) events log.

So the "correct" solution would be for JSONRPC over stdin/stdout, and keep stderr handy.

[stderr]  I[06:16:06.960] <-- textDocument/didOpen
[stderr]  I[06:16:06.976] System includes extractor: successfully executed /usr/bin/clang
[stderr]  	got includes: "/usr/lib/clang/16/include, /usr/local/include, /usr/include"
[stderr]  	got target: "x86_64-pc-linux-gnu"
[stderr]  I[06:16:06.977] --> textDocument/publishDiagnostics
[jsonrpc] e[06:16:06.996] <-- textDocument/publishDiagnostics 

(I'm the author of the Eglot LSP client for Emacs)

joaotavora avatar Jan 04 '24 12:01 joaotavora

Ah right you are stderr is used. llvm::errs() is the stream for stderr the message is being sent through.

On Thu, Jan 4, 2024 at 06:21 João Távora @.***> wrote:

The "correct" solution would be to understand how clangd checks it runs in terminal and modify how the process is being opened since that'd affect other processes similarly.

Just randomly popped here when checking out LEM. Are you sure clangd isn't really writing that message to stderr? Mine is. I think there's nothing in the spec against that. clangd writes lots of stuff to stderr, which one can use to make a (very useful, IMHO) events log.

So the "correct" solution would be for JSONRPC over stdin/stdout, and keep stderr handy.

[stderr] I[06:16:06.960] <-- textDocument/didOpen [stderr] I[06:16:06.976] System includes extractor: successfully executed /usr/bin/clang [stderr] got includes: "/usr/lib/clang/16/include, /usr/local/include, /usr/include" [stderr] got target: "x86_64-pc-linux-gnu" [stderr] I[06:16:06.977] --> textDocument/publishDiagnostics [jsonrpc] e[06:16:06.996] <-- textDocument/publishDiagnostics

(I'm the author of the Eglot LSP client for Emacs)

— Reply to this email directly, view it on GitHub https://github.com/lem-project/lem/issues/1076#issuecomment-1877009360, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAT2P6EV4HZISS6LX4HH4XLYM2NDTAVCNFSM6AAAAAA4RHHEEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZXGAYDSMZWGA . You are receiving this because you authored the thread.Message ID: @.***>

resttime avatar Jan 29 '24 04:01 resttime

Here is a solution, this gets rid of stderr:

(define-language-spec (c-spec lem-c-mode:c-mode)
  :language-id "c"
  :root-uri-patterns '("compile-commands.json")
  :command `("bash" "-c" "clangd 2> /dev/null")
  :install-command ""
  :readme-url ""
  :connection-mode :stdio)

garlic0x1 avatar Jan 29 '24 08:01 garlic0x1