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

Discussion: LSP-server readiness indicator #511

Open
ljw1004 opened this issue Jun 29, 2018 · 12 comments
Open

Discussion: LSP-server readiness indicator #511

ljw1004 opened this issue Jun 29, 2018 · 12 comments
Labels
feature-request Request for new features or functionality new request
Milestone

Comments

@ljw1004
Copy link
Contributor

ljw1004 commented Jun 29, 2018

How do you fellow LSP-server-authors report the readiness of your LSP-servers? I propose we add a standard for this to LSP. The Java extension displays in the status-bar one of three states: in-progress, ready or failed:

IN-PROGRESS:
image image

READY:
image

FAILED:
image

Omnisharp also displays its own readiness icon. (I think it displays progress and failure, but I haven't been able to get it right now).
image

In general (1) users want to know about LSP-server-readiness. They want to know e.g. that find-all-refs or autocomplete won't work yet but will soon. (2) For complex LSP servers and for large projects, there will be delays that take 30+ seconds where we should display progress, and there will be myriad failure modes that should be displayed to the user.

What do you reckon? Is it okay that each language extension builds its own ad-hoc display in the status-bar?

Here's what we did. I'll show the video first, and then explain why...
language-status-proposal

At first we did it just like Java and Omnisharp, each LSP server displaying its own ad-hoc status indicator in the status bar. But we've found ourselves using several LSP servers for a single document, each serving different aspects of functionality. And so the user ended up with four identical thumbs-up icons in the status bar, which was pretty unfriendly -- takes too much room, and the user has to hover to see which one is which.

  • Flow LSP server provides diagnostics, autocomplete, definition, find-refs, ...
  • ES-Lint LSP server provides linting diagnostics, and quick-fixes for them
  • JS-Imports LSP server provides autocomplete for imports statements, and quick-fixes for when you use symbols that need to be imported
  • GraphQL LSP server provides diagnostics and autocomplete specifically for GraphQL APIs.

Next we tried combining them into just a single icon, which displays the "worst" status of all current LSP servers (e.g. if two are thumbs-up, and one is failed, then display only the failed one). But this was a poor UX because you too often didn't get to see what was important to you.

And even despite displaying status, we continued to get 2-3 bug reports a week from people saying "autocomplete isn't working" or "hover isn't working", at times when the language server was in-progress loading or had failed, but the status bar was too subtle for them to notice.

That's why we implemented the more standardized uniform approach in the video above. Our goal was that users should feel more knowledgeable about their LSP server status (which they need and want to know), and should be more empowered to diagnose+fix issues themselves. We implemented it with a new LSP request, window/showStatus, sent from server to client:

// window/showStatus
// params: ShowStatusParams
// return: either a MessageActionItem if the user clicked a button, or null.

export type ShowStatusParams = {
  type: number,  // same as in ShowMessageRequest, "Info/Warning/Error"
  actions?: Array<MessageActionItem>, // buttons to show
  message?: string, // displayed in the tooltip when user hovers
  shortMessage?: string, // an optional short message to display in the status tab
};

What do you folks reckon? Who else has wanted to display LSP-server readiness status? How are you displaying it? Do you wish for a standardized way to display it? How is your ad-hoc status display holding up in cases of several LSP servers being used on the same document?

@jeanlucburot
Copy link

In my opinion the language server readiness is for people developing the IDE, but not for developers using it. I do get the IDE developers view, spending a lot of effort to make an LSP work in the IDE and then showing all kinds of status. The thing is that conceptually the language server is not very interesting for the IDE users, but rather the advantage to get programming language specific functionality. If a developer opens a new window i.e. for writing something in Java, he will want to type some stuff and get a response for it. It is quite irrelevant for the user whether there is some server of any kind behind or not. While the language server is not ready I think it is best to just show a loading bar that disappears uppon completion, somewhere within the newly opened window. Verbose status information about the language server is in my opinion only interesting now, where the LSP project is reasonably new, but I see the gain in having flexible, portable, and transparent language specific information for my IDE, rather than blowing it up with additional info that the end user will have to learn and understand about.

Summing up, I would agree to have some standardized way of getting info about the readiness of the language server. In LSP4J you get language/status notifications for that. Best would not be a message, rather have a simple integer ranging from 0 to 100 showing the % of the language server readiness. What message is displayed is displayed or what behaviour is implemented is up to the IDE developers, and shouldn't be decided by the LSP.

@mickaelistria
Copy link

FWIW, Eclipse LSP4E in Eclipse IDE shows a standard progress report indicator when the LS is starting. This is an implementation choice done by the client, and I think it's how it's best. As we don't use an icon for the language-server in Eclipse IDE, we show it plain text a bit like JDT.LS does in VSCode. Using an icon or not is IMO also better as a client-side decision.
But there is no tracking of actual progress (it's a binary working/done state) as the LSP doesn't specify a standard way to report progress. This is the topic of #70. Once there is a generic way to report progress for any request, then the LS could send progress report for initialize and each client should be responsible of rendering it as it prefers (progress bar, percentage, playing a movie...).

@kdvolder
Copy link

kdvolder commented Jul 3, 2018

I recall there was an issue raised somewhere already about progress reporting. I think we need something like that for when a language server needs to do potentially long-running background work that may interfere with normal functioning of the language server (e.g. downloading of dependencies, while this is in progress one might expect that linting and/or completions do not work).

I don't think this is necessarily connected to showing status of the language server startup. Yes, it is common for a lot of this work to happen at startup, but it could also happen anytime (e.g. when a server detects a change in the project that requires lengthy operations to update some internal models of the project).

I also think it is important for the user to know when something in the server has gone wrong that could interfere with server's functionality (e.g. a problem resolving dependencies). But perhaps there is no need for a special mechanic for this. Server could probably just report these types of problems via the existing 'Diagnostic' markers that is used also for linting.

@keegancsmith
Copy link

There was discussion about progress in this PR #245

@matklad
Copy link
Contributor

matklad commented Jul 1, 2020

We also need status for rust-analyzer (and we‘ll go with ad-hoc status bar). It‘s important to separate the notion of transient progress and notifications (for which we already have great support) from the notion of the status (which generally doesn’t change until the user makes some action).

A specific thing for which we need status indicator is „project manifest has changed, do you want to reload project?“ While we can do auto-reload behind the user‘s back, we’d rather not to, as project reload might trigger network access (and even arbitrary code execution). Status indicator would be a perfect place to show the user that something’s not completely fine, and to provide the user-triggered reload action.

@puremourning
Copy link

I'd back this change. Currently in YCM we have to have custom logic to parse the jdt.ls messages to work out when the server will actually respond to requests. This is made a ton worse for automated testing. If the server had a true "ready?" request or "ready!" notification, then we wouldn't have to guess or write server-specific code to work out when our tests (or users) can expect requests to actually be processed.

@dbaeumer
Copy link
Member

dbaeumer commented Jul 2, 2020

The initialize request does allow to report progress since 3.15 if the client sets a progress token. The VS Code LSP client supports that.

@matklad in your scenario there is an explicit user action. Right?

If a server receives a request and it is not ready to serve the request IMO the best to do is that clients setup progress tokens which then can be used by the server to reports this. In an async world I don't see how a ready notification / request from the server can make this fail save.

@puremourning
Copy link

The initialize request does allow to report progress since 3.15 if the client sets a progress token. The VS Code LSP client supports that.

Oh that's interesting, thanks. however, unless i'm mistaken that requires the actual 'initialise' response to be delayed until the server is "really" ready?

I think most servers simply reply immediately to the initialise response then use some out-of-band mechanism to report readiness. Example i guess would be something like clangd building its index (they have a custom message for this) and jdt.ls loading the eclipse projects (again, custom message). In truth the server is 'initialised" and will process requests after the initialise response, but it's not really "fully ready" until all the indexes are up to date.

I realise it might not be simple to canonicalise these states in a completely generic way, but perhaps server vendors could make use of a set of predefined states for these use cases. Certainly, there's enough real-world examples of custom approaches to suggest that there's a meaningful use case there.

@matklad
Copy link
Contributor

matklad commented Jul 2, 2020

@matklad in your scenario there is an explicit user action. Right?

@dbaeumer there's an explicit user action for changing status: the project will be in "needs reload" state until the user does a reload

@robertoaloi
Copy link
Contributor

We have a similar situation in the Erlang Language Server in relationship to the initial indexing procedure.

At the beginning (when progress reports were not available), we were waiting for the indexing to complete before replying with an initialize. That was, as you can imagine, not a nice user experience (long wait).

Later on, we moved the start of the indexing to the initialized request and reported progress via workdone_progress_reports. This is providing users a better experience, but still not ideal. They get a visual indication that something is still going on, so they somehow expect that some functionalities still work in degraded mode.

What would be much better is for the protocol to allow us to notify that we are still not "fully ready". Maybe the server could send back, where appropriate, a clear status code (not ServerNotInitialized, but something similar).

Thoughts?

@matklad
Copy link
Contributor

matklad commented Apr 6, 2021

Drafted a relatively server-independent extension on rust-analyzer side: https://github.com/rust-analyzer/rust-analyzer/blob/ad02bfe58fd52293d9ae4a049f98f475df9d3abb/docs/dev/lsp-extensions.md#server-status. The purpose of that API is primarily informing the user. That is, if the server doesn't work because the build is broken, persistent serverStatus is the right UX to surface this info as a user.

The extension doesn't try to solve the problem of informing the client when it might be not appropriate to send request. For that, we employ a simple hack of answering with ContentModified, which causes the client to ignore the error: https://github.com/rust-analyzer/rust-analyzer/blob/ad02bfe58fd52293d9ae4a049f98f475df9d3abb/crates/rust-analyzer/src/main_loop.rs#L481-L490. That's a hack, but, if you squint hard enough, it even makes some sense semantically -- the server refuses to respond because it knows that the response will be immediately obsoleted by indexing results.

@matklad
Copy link
Contributor

matklad commented Aug 11, 2021

Additional observation: server initialization can fail due to something being misconfigured on the user's side, and such misconfiguration can usually result in lengthy error messages. It would be great if editor UI for status would afford displaying a verbose error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality new request
Projects
None yet
Development

No branches or pull requests

9 participants