Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggling the lsp-mode does not actually toggle the server #77

Closed
rski opened this issue May 4, 2017 · 21 comments
Closed

Toggling the lsp-mode does not actually toggle the server #77

rski opened this issue May 4, 2017 · 21 comments
Assignees

Comments

@rski
Copy link
Contributor

rski commented May 4, 2017

If lsp-mode was active, doing M-x lsp-mode (toggle off) M-x lsp-mode (toggle on again) doesn't try to stop and then start the server again.

As a result of this, if the server for some reason dies, it is not possible to restart it in the current project unless you restart emacs or muck with variables.

@vibhavp
Copy link
Member

vibhavp commented May 4, 2017

Ah, never thought of that. I'll try adding toggle functionality, thanks

@alanz
Copy link
Contributor

alanz commented May 6, 2017

Also, see the approach in https://github.com/commercialhaskell/intero/issues/364

There is both a blacklist and a whitelist

@alanz
Copy link
Contributor

alanz commented May 6, 2017

Perhaps toggling lsp-mode in the presence of the lsp-project-blacklist should offer to add it to the lsp-project-whitelist.

@Alexander-Miller
Copy link

it is not possible to restart it in the current project unless you restart emacs

Not even closing emacs works. It tries to shut down the lsp process, sees that's it's not live, and throws an error (this happens in lsp--stdio-send-(a)sync). I've had to use xkill to exit emacs when that happened.

@astahlman
Copy link
Contributor

I believe the error that @Alexander-Miller referenced is triggered in the process sentinel, here:

(dolist (buf (lsp--workspace-buffers lsp--cur-workspace))

The function references lsp--cur-workspace, but by the time the sentinel runs, the workspace has already been set to nil, here:

(setq lsp--cur-workspace nil)

Regarding the original proposal: correct me if I'm wrong, but wouldn't toggling the minor mode in a single buffer affect all buffers associated with the project workspace? In other words, if buffer A and buffer B are both visiting files under the same project root directory, and I toggle lsp-mode in buffer A, then the language server would "disappear" from the perspective of buffer B.

Is that what's intended here? As a user, I think this would be surprising and not very intuitive. It seems cleaner (albeit more difficult) to decouple the lifecycle of the language server from the status of the minor-mode in each buffer.

@astahlman
Copy link
Contributor

@alanz I see that you were working on this in #82. Any comment on the relationship between the the language server process and the individual documents in a workspace?

IMHO, the relationship between:

  • the language server process(es)
  • the lsp--client(s)
  • the workspace(s)
  • each document in a workspace
  • major and minor modes

...is a little unclear and could use some more thinking. Perhaps a good first step would be for someone to document the ideal state?

@phst
Copy link
Contributor

phst commented Oct 7, 2017

I think the ideal state would be as follows: Any buffer could have any number of servers associated with it. That is, a buffer could have zero, one, or more servers, and multiple buffers could talk to the same server. The association should not be based on workspace root directories or modes, but completely arbitrary, because the association is intrinsically server-specific. For example, it's possible that a server could be used for an arbitrary number of languages and major modes, and vice versa a buffer should be able to use different servers providing different functionality.

@alanz
Copy link
Contributor

alanz commented Oct 10, 2017

Sorry for the long delay, I had some ISP issues.

For me, focusing on a single language server (haskell), the relationship is simple.

An IDE may open multiple projects.

A "project" is a thing that makes sense for the given dev context, and is normally well enough understood.

Each project has a single language server running. For haskell this is quite important, as a server can use up quite a bit of resources.

A workspace corresponds to a project.

A document belongs to a workspace/project, and all documents for a project are handled by the same server.

I do not have a strong opinion on the major/minor thing. Certain features can be exposed piecemeal, like flycheck, and the company stuff.

It does make sense for it to be fully integrated into a given mode though.

A bit of a ramble, I am afraid.

@astahlman
Copy link
Contributor

@phst Do you have an idea how the life-cycle management of the language servers would work if the association between buffers and language servers were arbitrary and decoupled from the workspace?

I like the approach @alanz suggests of a 1:1 correspondence between workspace and language server where a document belongs to a single workspace because it's simple and easy to reason about. But I also foresee issues with projects that encompass multiple languages, e.g., a Haskell-based web application that includes source files in Haskell, HTML, CSS, and Javascript, bash, make, etc.

@vibhavp
Copy link
Member

vibhavp commented Oct 11, 2017

The issue with projects using multiple languages can be addressed by letting many buffers share the same workspace struct object, which can be accomplished by using a global hash table.

@vibhavp
Copy link
Member

vibhavp commented Oct 11, 2017

On second thought, I'm not sure if that is a good idea. lsp-after-change uses the workspace object to send the changes to the language server, and doing a hash lookup after every textual change to the buffer might result in slowdowns, especially in a project with a large amount of files open.

@Alexander-Miller
Copy link

Do you really need hashes? Why not store a buffer's workspaces as a buffer-local variable right in the buffer?

Also does the LSP really require to send an event on every change? I think an idle timer would be a simpler solution.

@alanz
Copy link
Contributor

alanz commented Oct 11, 2017

We used to use an idle timer. But then we discovered that we can't batch the changes. See d696926

@vibhavp
Copy link
Member

vibhavp commented Oct 12, 2017

Why not store a buffer's workspaces as a buffer-local variable right in the buffer?

That's what we currently do, see lsp--cur-workspace

@phst
Copy link
Contributor

phst commented Oct 29, 2017

Why not store a buffer's workspaces as a buffer-local variable right in the buffer?

That's what we currently do, see lsp--cur-workspace

Could you extend that variable to a list of workspaces so that each buffer can have more than one workspace?

@phst phst mentioned this issue Nov 16, 2017
@colonelpanic8
Copy link

It seems that the discussion here ended up going off on a tangent about associating buffers with lsp server processes.

What is the status of the original issue being unable to get emacs to restart the server process? That seems like it should be easy enough...

I am aware of lsp-restart-workspace, but this does not seem to work (at least for lsp-haskell).

@alanz
Copy link
Contributor

alanz commented Apr 6, 2018

@IvanMalison please give details about it not working for lsp-haskell. That is what I use, and it works for me. And is needed because of the memory leak in the haskell LSP server.

@yyoncho yyoncho self-assigned this Oct 19, 2018
@yyoncho
Copy link
Member

yyoncho commented Oct 28, 2018

@MaskRay IMO we should merge lsp--management-mode and lsp-mode which will fix that issue.

@MaskRay
Copy link
Member

MaskRay commented Oct 29, 2018

@yyoncho LG. lsp--manged-mode and lsp-mode


lsp--managed-mode currently means a buffer that has sends a textDocument/didOpen notification

@vibhavp
Copy link
Member

vibhavp commented Oct 29, 2018

On second-second thought, lsp-mode is a lot like ERC. Even though there is an erc-mode, turning it on or off doesn't connect/disconnect you to an IRC server. The primary entrypoint for ERC is M-x erc, not M-x erc-mode , just like lsp--start. IMO, a better way to achieving some toggling functionality would be to add lsp-toggle.

@yyoncho
Copy link
Member

yyoncho commented Oct 29, 2018

@vibhavp I second that - after #424 is implemented we might ask which server to toggle if there are multiple servers for the current file. We may consider also the name lsp-unlink.

yyoncho added a commit to yyoncho/lsp-mode that referenced this issue Nov 28, 2018
Fixes emacs-lsp#462 emacs-lsp#444 emacs-lsp#424 emacs-lsp#419 emacs-lsp#392 emacs-lsp#390 emacs-lsp#376 emacs-lsp#360 emacs-lsp#359 emacs-lsp#340 emacs-lsp#335 emacs-lsp#333 emacs-lsp#285 emacs-lsp#280 emacs-lsp#229
Fixes emacs-lsp#225 emacs-lsp#153 emacs-lsp#111 emacs-lsp#117 emacs-lsp#140 emacs-lsp#77

Higher level goals:

* Allow multiple servers to work in one project and in one file
* Make finding of project roots easier and more explicit for the users.
* Removed all synchronous calls from the server startup.
* Improved hadling of status messages.
* Implemented workspace folders support as a first class citisen instead of
  being a patch in the existing configuration.
* Simplified the code as much as possible

The following things are implemented:

1. Moved `lsp-mode` related code into single file. Now clients should either
require `lsp-mode` or `lsp`. After this CL `lsp-mode.el` and `lsp.el` extesions
like `ccls` and `lsp-ui` will be able to work against either of these.
`lsp-mode` will be used by default and if you want to use the new `lsp.el` you
should do `(require 'lsp)` before loading lsp-mode.

2. Introduced `lsp-session` which will be resposible for handling project
session configuration like:
* What currently imported projects.
* What server are running and the folders that are associated with the session.
* What folders are ignored and wont be used to initialize a project in it. The
session is persisted and automatically loaded the first time user calls `lsp`

3. Introduced `lsp-language-id-configuration` which will hold the configuration
"language" -> emacs major mode. This map is used to render eldoc information and
also to determine the language id when sending various notifications to the
server.

4. Restructored `lsp--client` by removing the runtime related
stuff (like last-id) so it can be used as a template for registration. This will
simplify the way lsp client registration and it will make easier to add new
functionality

5. lsp-ui and company-lsp should be adapted as well. I will prepare separate CLs.

6. Implemented frontend for reviewing what servers are running, which buffers
are managed, and so on the method have replaced the existing `lsp-capabilities`
method.

7. Reworked the modeline so it can provide info for the servers currently
handling current file.

8. Moved snippet configuration into `lsp-mode` from `company-lsp` since snippet
support will be needed as part of implementing snippet support in lsp-mode emacs-lsp#350.
Aa a result it was possible to clean registration related methods since they
where used only by company-lsp.

9. Added the option each client to define the location of its libraries and thus
avoid starting LSP servers for library folders.

10. Method selection based on the configuration in `lsp-method->capabilities`.

Sample client implementation:

``` emacs-lisp
(lsp-register-client
 (make-lsp-client
  :new-connection (lsp-stdio-connection "pyls")
  :major-modes '(python-mode)
  :server-id 'pyls
  :library-folders-fn (lambda (_workspace)
                        (list "/usr/"))))
```
@yyoncho yyoncho closed this as completed Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants