Tolerate "processId":null that may be send by Emacs LSP client#1
Closed
michaliskambi wants to merge 1 commit intoIsopod:masterfrom
Closed
Tolerate "processId":null that may be send by Emacs LSP client#1michaliskambi wants to merge 1 commit intoIsopod:masterfrom
michaliskambi wants to merge 1 commit intoIsopod:masterfrom
Conversation
michaliskambi
added a commit
to michaliskambi/elisp
that referenced
this pull request
Nov 10, 2022
Isopod
added a commit
that referenced
this pull request
Nov 10, 2022
Fixes a bug with JSON parsing See also: #1
Owner
|
Thanks for your investigation and the patch! You have actually discovered a bug in my json library: Values that are not consumed are supposed to be skipped. So in this case, since we are not explicitly handling "processId", the value should just be ignored and parsing should continue with the next item. But this did not work correctly for null values. It should be fixed now in master, so I'm closing this. Please reopen if you still get the error. |
Author
|
Cool, I see Isopod/jsonstream@c9601be and confirm that it also fixes the issue (both in my test program |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Emacs LSP client sends the "initialize" JSON request containing
nullin place of theprocessId:(The full sample JSON send by Emacs client is in the attached test application.)
This causes JSON reading in
Initializeto fail to read the following information, in particular it fails to read theRootUri. More precisely, the loopends prematurely, before we've actually seeen all parameters. Briefly looking at
TJsonReaderimplementation, I understand that the expectation is that one should callReader.Nullin case the value may benullin JSON, to allowTJsonReader.Nullto doStackPop; Reduce;.I'm attaching
test_json.lpr, a simple JSON reader test (using onlyJSONStreamunit, nothing else). I used this to confirm the issue and test the solution. It contains hardcoded JSON requests I logged from Emacs and VS Code (for comparison) LSP clients. If you comment out theinside and run it, you will get:
So in case of Emacs JSON, the only
keyfound inparamswasprocessId.Uncommenting the code doing
Reader.Null, we get the desired result, androotUriis correctly parsed for both VS Code client and Emacs client:test_json.lpr.txt
The lack of
RootUriseems to have dire consequences for pasls: It reports then that it cannot find units from LCL, likeLaz_AVL_TreefromLazUtilspackage, in my test on michaliskambi/elisp#1 . Despite the log showing it can find thelazutils.lpkcorrectly. In effect, code completion in Emacs with this LSP server was failing for me when some LCL unit likeLaz_AVL_Treewas used. I didn't analyze further (how lack ofRootUriis causing this, despitelazutils.lpkbeing found), but makingRootUricorrectly detected fixed everything :)Side investigation: Why does Emacs LSP client send
"processId":null?I found it clearly done in the lsp-mode source code: They literally pass
:processId nilin https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L7480 . There's no attempt to initializeprocessIdto anything non-nil, nor any comment why.Curiously, I run
git blameon it, and found this commit: emacs-lsp/lsp-mode@e7c7abf that exactly changes it from valid Emacs PID to a hardcodednil.It leads to sending nil pid emacs-lsp/lsp-mode#1408 which leads to Intermittent connection in typescript-language-server via Docker-tramp emacs-lsp/lsp-mode#1240 for reasoning. Looks like in some scenarios with Emacs+Docker, the PID would be invalid, as the LSP server would be in a container and LSP client (Emacs) PID would come from outside. The invalid PID would in turn cause some LSP servers to exit. So it's better to send a
nilPID than to allow LSP servers to rely on it.