Add support to report progress for a running request

[dbaeumer]

No description provided.

[felixfbecker]

Moving my suggestions here

I see two possibilities here

• Go with the pattern of HTTP 202 Accepted. You send a request, and get back a response instantly stating the request has been "accepted". You can then do more requests with a different method, like $/progess (similar to cancel), passing the message ID of the initial request as a param, and get progress reports back.
• Go with a pattern similar to HTTP 102 Processing (WebDAV). You send a request, and before the actual response comes in, the server can decide to continuously send a Progress header, and finally the actual Content-Length header and response body.

[felixfbecker]

Proposal

Add a new notification window/progress that is sent from the server to the client.
Params:

interface ProgressParams {

  /** a unique identifier to associate multiple progress notifications with the same progress */
  id: string | number;

  /** 
    * an optional request ID, indicating that this is the progress of a specific request from the client
    * if set, the client can use the information to present a progress indicator in the context of the action that triggered the request
    * if not set, the progress is treated as arbitrary progress for an arbitrary async operation (for example indexing) and the client can show it in a general location, like a status bar
    */
  requestId?: number | string;

  /** number that ranges from zero to total */
  status: number;

  /**
    * optional total number that status is relative to.
    * If set, the client can choose to display this next to the progress indicator.
    * If not set, assumed to be 1.
    */
  total?: number;

  /**
    * optional label for the progress.
    * If the client decides to use it, it should be displayed together with the progress status
    */
  label?: string;
}

It can be used to report progress for requests:

-> textDocument/references request (id 1)
<- window/progress notification (progress id 1, request id 1, status 0.5)
<- window/progress notification (progress id 1, request id 1, status 1)
<- textDocument/references response (id 1)

It can also be used to report progress for file indexing for example:

<- window/progress notification (progress id 2, status 1, total 1264, label "indexing file hello.php")
<- window/progress notification (progress id 2, status 134, total 1264, label "indexing file world.php")
<- window/progress notification (progress id 2, status 1264, total 1264, label "indexing file whatever.php")

A progress indicator should be hidden by the client once a progress with status == total comes in.

[kaloyan-raev]

Should we have some feedback from the client to server if it was able to process a certain progress status?

Imagine, like in the indexing, that the server floods the client with tons of progress status messages. If the system is not fast enough, we may have the situation where the server is notifying for progress status 1000 while the client still handles status 50 and has 950 more in the message queue.

The above situation can be mitigated if the server sends a new progress status only if the client was able to handle the previous one. This way the server will send only a fraction of all possible progress statuses, being gentle to the client.

[felixfbecker]

Well the notifications are like events, they don't get a response. The client can choose the handle them or not. In a way they are like UDP, we just send out how the progress is going. The server needs to make sure he only handles the newest one, similar like if you sent the current time over UDP every second, or doing VoIP. A client would not queue these, but just discard messages he could not handle.
Using a request/response scheme for this would be unneeded overhead for this, the server needs to generate a unique ID for every request and map the responses back once they come in. You avoid that with notifications, as there is no real "result" from the client in this case and the server is not interested in whether the client could handle the notification.

[felixfbecker]

Of course ClientCapabilities should be extended so a server can save sending these notifications if the client has no use for it.

[kaloyan-raev]

Although it is just a notification it can really cause a lot of overhead. Even just discarding the notifications can be expensive if the messages are thousands per second.

I like the idea of utilizing the ClientCapabilities. Perhaps, the client can tell the server about the granularity of the progress information that makes sense to be sent. For example, if the client displays the progress in percentages then no more than 100 notifications are really necessary (instead of tens of thousands).

[felixfbecker]

Sure, but using request/response would add even more overhead.
The number is not really important, it's the rate - in our use case, when we index 1000 files, I would send 1000 progress notifications, and that would be okay because it would be over a timespan of over a minute.
I doubt any server would have so granular progress indications for something simple like a references request that it would be a problem. For example, if the server knows he needs to iterate x ASTs, he can send a progress notification after every iterated AST. Basically I would shift that responsibility to the server, he should be responsible for not spamming notifications after every CPU tick (that would be bad for the server performance too, not just the client)

[kaloyan-raev]

Sure, but using request/response would add even more overhead.

If we can take advantage of the ClientCapabilities then request/response would not be necessary anymore.

The number is not really important, it's the rate - in our use case, when we index 1000 files, I would send 1000 progress notifications, and that would be okay because it would be over a timespan of over a minute.

It is exactly the rate that worries me. At the moment, on my machine, the PHP Language Server indexes ~ 150 files per seconds. It's too much to send a progress notification 150 times per second.

[felixfbecker]

It is exactly the rate that worries me. At the moment, on my machine, the PHP Language Server indexes ~ 150 files per seconds. It's too much to send a progress notification 150 times per second.

Actually, we already send those. We send a logMessage notification for each file. Haven't noticed any problems with this really, but we could easily choose to report only every 10th file or so.

[kaloyan-raev]

Actually, we already send those. We send a logMessage notification for each file. Haven't noticed any problems with this really, but we could easily choose to report only every 10th file or so.

I did some performance measurements. The test case was the PHP language server to index its own source code. The performance penalty of sending a logMessage for each file is around 3%. This is on the server side. There should be an additional performance penalty on the client side that I cannot measure right now.

We can have a long debate if this performance penalty is significant or not. For other use cases with other language servers, the performance penalty of such intensive progress reporting can be significantly lower or higher. Also, different users have different perception what performance penalty is acceptable or not.

This is why most software solutions make logging configurable. You can switch it and off. You can set the a log level (like only error messages, but no warnings and info messages). BTW, this is something we miss in the LSP - the client should be able to tell if logging is desired and at what level.

Same for progress reporting - the client should be able to tell if progress reporting is desired and at what rate. Some clients may want a progress progress not more frequently than 1 seconds, others than 100 ms, or 10 ms. There could be clients that want it all, or not at all.

It should not be a big deal to incorporate such progress reporting rate in the ClientCapablities and have servers respecting it.

[felixfbecker]

I did some performance measurements. The test case was the PHP language server to index its own source code. The performance penalty of sending a logMessage for each file is around 3%. This is on the server side

For the record, PHP is not the fastest, and my implementation probably isn't either.

Same for progress reporting - the client should be able to tell if progress reporting is desired and at what rate. Some clients may want a progress progress not more frequently than 1 seconds, others than 100 ms, or 10 ms. There could be clients that want it all, or not at all.

I'm all for having a flag in ClientCapabilities to enable/disable progress reports.

It should not be a big deal to incorporate such progress reporting rate in the ClientCapablities and have servers respecting it.

It complicates the implementation a bit. For basic progress reporting, we just have to increment a counter together and report it with total after each parse. With rate limiting, we have to hold a timestamp of the last progress report, measure if the difference, and only report if the difference is greater than the rate limit.

Proposal for ClientCapabilities in that case:

interface ClientCapabilities {
  /**
    * The client provides support for window/progress notifications
    * If a number, specifies a rate limit as the minimum number of milliseconds between two progress notifications.
    * If true, there is no rate limit.
    */
  progressProvider?: boolean | number;
}

[kaloyan-raev]

How about using an additional ProgressOptions interface like CompletionOptions is used for the completionProvider?

/**
 * Progress reporting options.
 */
interface ProgressOptions {
  /**
   * The minimum number of milliseconds between two progress notifications.
   */
  rateLimit?: number;
}

interface ClientCapabilities {
  /**
   * The client can process window/progress notifications.
   */
  progressProvider?: ProgressOptions;
}

This way it is easy to extend the protocol with additional options without breaking compatibility. It will also avoid the issue described in #39.

[felixfbecker]

So null or undefined would then mean no progress support

[kaloyan-raev]

It should be the same semantic like the providers in the ServerCapabilities.

null or undefined for the progressProvider would mean no progress support, i.e. the server should not send any progress notifications.

null or undefined for the rateLimit should mean no rate limit, i.e. the server should send all progress notifications without any limitation. Naturally same should happen if rateLimit is 0 or negative.

[bruno-medeiros]

It complicates the implementation a bit. For basic progress reporting, we just have to increment a counter together and report it with total after each parse. With rate limiting, we have to hold a timestamp of the last progress report, measure if the difference, and only report if the difference is greater than the rate limit.

That's only a few extra lines of code, it doesn't seem that much of a hurdle.

[martinring]

The rate limit makes no sense to me. It would have to be introduced in various other places as well after that logic. publishDiagnostics is far more complex and is not rate limited. It should be clear, that a language server is not to send hundreds of thousands progress messages per second, just as a language server does not send hundreds of thousands diagnostics per second.

Also introducing a rate limit as a static client capability seems a bad solution to me

• What happens if the server does not respect the rate limit?
• How would you even determine a static value for a rate limit. It depends on the hardware capabilities as well as other software running.

If something like that should be introduced I would rather suggest a global (symmetrical) back-pressure mechanism on the messaging level. But again that would make implementations more complicated.

As a last point:

There is no added value for a user if the rate of any message is above say 50 per second, since that is not observable for humans anymore. For any client that kind of message rate seems no problem at all so I would just suggest to add a design guideline for server implementations.

[felixfbecker]

I share @martinring's opinion. Servers should just treat the rate of sending notifications with common sense. In 99% of the cases this will probably not cause a problem and if it does an issue should be opened at the LS repository that it should overthink the points at which the notifications are sent.

[kaloyan-raev]

The rate limit makes no sense to me. It would have to be introduced in various other places as well after that logic. publishDiagnostics is far more complex and is not rate limited. It should be clear, that a language server is not to send hundreds of thousands progress messages per second, just as a language server does not send hundreds of thousands diagnostics per second.

Yes, publishDiagnostics is not rate limited. But it does not means we should not think about performance optimizations there too. @martinring, you are just assuming that language servers do not send hundreds and thousands of publishDiagnostics notifications per second to the client. But you don't really know. In fact, currently the PHP language server sends a publishDiagnostics notification whenever a PHP file is parsed. This means that during indexing there is a publishDiagnostics notification sent for each PHP file. These are another 150 messages per seconds in the pipe. Does it make any sense? Maybe. There are IDEs like Eclipse that want to display every possible diagnostic in a dedicated Problems view. But most of the other IDEs and editors are only interested in diagnostics for the files that are currently opened. How does the server know if the client is interested in diagnostics for all project files or only for the opened ones? It does not. Any assumption has a high chance to be wrong. The client should be able to tell the server what is expected.

Same for log messages that I mentioned in a previous comment. The client should be able to tell the server what is the expected log level of log messages.

Following the same logic, we should have some way to limit the number of progress notifications.

What happens if the server does not respect the rate limit?

Perhaps, nothing fatal would happen. Just the communication between the client and the server would be wasteful. This may result in noticeable issues with responsiveness in the IDE in certain situation. It may become more severe if the client and the server are not on the same host and networking between them is expensive.

How would you even determine a static value for a rate limit. It depends on the hardware capabilities as well as other software running.

I don't think it depends that much on the hardware capabilities rather than on the design goals of the client. Some clients would want to display the progress in a smooth way and they would want something like 30 progress updates per seconds, i.e. rate limit of 30 ms. I would expect most clients to want just a few refreshes per second, i.e. rate limit of 200-500 ms. On the other extreme would be clients that connect to the server over the wire and networking is very expensive. They would want to limit the the progress update to once every 3 seconds, i.e. rate limit 3000 ms.

As you can see there is 100 times difference between the two extremes. How could the server assume a fixed rate limit value that fits all client? Isn't it best to let the client tell it?

The LSP was designed after VSCode. There are lot of assumptions done that are closely related to the VSCode implementation. Most of issues opened are for clarifying these assumptions and even trying to get rid of some of them. The more clients try to adopt LSP, the more assumptions will be challenged.

I suggest to avoid making some assumption in this case. Let's not assume that progress notifications are cheap and that the client and the server are running on the same host.

[martinring]

@kaloyan-raev I agree, that publishDiagnostics should get more information about what information is desired, e.g. though view perspectives (right now the only information on the server side is which files are open, not which files are looked at and where). But this is completely off-topic.

A rate limit seems just a very bad soultion to me. I don't take your argument about the network connection since a server should adapt to slow network individually. (slow network might be a temporary condition) There exist established solutions for this. In this case messages could be combined (or dropped) to account for slow network, which would (for a reasonable server) result in less progress messages beeing sent. But again this is nothing you can point out up front.

Is there even a real problem anywhere? I think this should be discussed, when a client exists or is beeing developed which cannot handle more than x progress messages per second. (I cannot imagine that x is below say 100 even for the most inefficient implementations) Otherwise this discussion seem a little pointless.

[kaloyan-raev]

Is there even a real problem anywhere?

Yes, there is. Wasteful CPU utilization and responsiveness lag, which I observe with the PHP language server. In a previous comment I mentioned that I measured 3% performance penalty, because of the use case with 150 notifications per second. And this is not the most aggressive situation possible. Imagine a file copy or ZIP extract operation that reports a progress on each file processed. This can easily grow to thousands of progress notifications per second.

A full pipe with garbage notification delays the rest of messages, which hits responsiveness in the editor during operation with intensive progress reporting.

[felixfbecker]

I think this is somewhat premature optimization. We should add progress reporting to the protocol, and if any real performance issues occur, we can design a solution for it that fixes the problem for all requests in a universal way, not just progress. From our current LS the perf will not change, the method will just change from logMessage to progress.

[felixfbecker]

A full pipe with garbage notification delays the rest of messages, which hits responsiveness in the editor during operation with intensive progress reporting.

A client can keep seperate buffers for responses and notifications. Responses can have a higher priority. Requests are not delayed at the client because they are in a different stream.

[kaloyan-raev]

A client can keep seperate buffers for responses and notifications. Responses can have a higher priority. Requests are not delayed at the client because they are in a different stream.

The communication channel between the client and the server is still a single one. So the client needs to read and decode the message before determining if it is for the "responses" or "notifications" buffer. This will not solve the responsiveness issue. As I have already mentioned, even discarding unwanted messages is expensive.

Also, who says that requests are with higher priority than notifications? Yet another assumption. As I am typing in the editor, I would be more interested to see real-time diagnostics than real-time progress of some background operation. Both progress and diagnostics are notifications.

[martinring]

@kaloyan-raev your 3% performance penalty was measured on the server if I got you right. Why don't you just reduce the rate of progress messages in your server implementation. What is this to do with the client or the protocol?

[kaloyan-raev]

This was just an example measurement done in VS Code to back my thoughts with some real data. I am actually interested in other clients where each message has a higher cost.

I don't have my own language server implementation. I contribute to a couple of language server implementations and plan to consume even more. I can imagine tough discussions with each of the language servers for hardcoding the progress reporting to once or twice per second.

Hence it would be much better to have an established way for this client-server negotiation directly in the LSP.

[Xanewok]

To spark/further current discussion on the feature, I pushed a PR with @keegancsmith's proposed protocol extension and a reference client implementation for it in microsoft/vscode-languageserver-node#261.
Please tell me your thoughts, also how to improve upon it or maybe if it'd be feasible to include it in the protocol itself.

[dbaeumer]

I merged in @Xanewok work for progress into the vscode libraries as a proposed API. While implementing cancel support I separated the notification into start, report, done and cancel. Mainly because it was very hard to descibe in which cases which property combinations were valid. Was a lot easier having separate literals per request. The currently implemented proposal looks like this:

Reporting server task progress

Many tools are capable of performing some background task processing or data streaming. From a UX point of view, it's good to report both the fact that the tool is performing some background work, but also report the progress being made for it. To realize that and to provide a simple proposal based on which the feature can be later improved, the following additions are proposed:

Client Capabilities:

The client sets the following capability if it is supporting notifying task progress.

	/**
	 * Window specific client capabilities.
	 */
	window?: {
		/**
		 * Whether client supports handling progress notifications.
		 */
		progress?: boolean;
	}

Progress Start Notification

The window/progress/start notification is sent from the server to the client to ask the client to start progress.

Notification:

• method: 'window/progress/start'
• params: ProgressStartParams defined as follows:

export interface ProgressStartParams {

	/**
	 * A unique identifier to associate multiple progress notifications with
	 * the same progress.
	 */
	id: string;

	/**
	 * Mandatory title of the progress operation. Used to briefly inform about
	 * the kind of operation being performed.
	 *
	 * Examples: "Indexing" or "Linking dependencies".
	 */
	title: string;

	/**
	 * Controls if a cancel button should show to allow the user to cancel the
	 * long running operation. Clients that don't support cancellation are allowed
	 * to ignore the setting.
	 */
	cancellable?: boolean;

	/**
	 * Optional, more detailed associated progress message. Contains
	 * complementary information to the `title`.
	 *
	 * Examples: "3/25 files", "project/src/module2", "node_modules/some_dep".
	 * If unset, the previous progress message (if any) is still valid.
	 */
	message?: string;

	/**
	 * Optional progress percentage to display (value 100 is considered 100%).
	 * If not provided infinite progress is assumed and clients are allowed
	 * to ignore the `percentage` value in subsequent in report notifications.
	 *
	 * The value should be steadily rising. Clients are free to ignore values
	 * that are not following this rule.
	 */
	percentage?: number;
}

Progress Report Notification

The window/progress/report notification is sent from the server to the client to report progress for a previously started progress.

Notification:

• method: 'window/progress/report'
• params: ProgressReportParams defined as follows:

export interface ProgressReportParams {

	/**
	 * A unique identifier to associate multiple progress notifications with the same progress.
	 */
	id: string;

	/**
	 * Optional, more detailed associated progress message. Contains
	 * complementary information to the `title`.
	 *
	 * Examples: "3/25 files", "project/src/module2", "node_modules/some_dep".
	 * If unset, the previous progress message (if any) is still valid.
	 */
	message?: string;

	/**
	 * Optional progress percentage to display (value 100 is considered 100%).
	 * If infinite progress was indicated in the start notification client
	 * are allowed to ignore the value. In addition the value should be steadily
	 * rising. Clients are free to ignore values that are not following this rule.
	 */
	percentage?: number;
}

Progress Done Notification

The window/progress/done notification is sent from the server to the client to stop a previously started progress.

Notification:

• method: 'window/progress/done'
• params: ProgressDoneParams defined as follows:

export interface ProgressDoneParams {
	/**
	 * A unique identifier to associate multiple progress notifications with the same progress.
	 */
	id: string;
}

Progress Cancel Notification

The window/progress/cancel notification is sent from the client to the server to inform the server that the user has pressed the
cancel button on the progress UX. A server receiving a cancel request must still close a progress using the done notification.

Notification:

• method: 'window/progress/cancel'
• params: ProgressCancelParams defined as follows:

export interface ProgressCancelParams {
	/**
	 * A unique identifier to associate multiple progress notifications with the same progress.
	 */
	id: string;
}

[ljw1004]

@dbaumer Should we be using this progress mechanism for the LSP server to indicate a long-running process of generating diagnostics? Or should that await some future LSP message?

Context: Users have a keen wish to know when a long-running diagnostic-generating operation has finished. I imagine that progress-of-computing-diagnostics would naturally be surfaced in the UI with in the "Issues" tab itself, or within the "issues count" icons that appear in the status bar. I don't know if you expect these progress messages to typically be surfaced elsewhere...

[dbaeumer]

@ljw1004 since publish diagnostics is a push model the server should make use of the new API if wanted. If we find out that the server needs to define a location we can add something later on.

What I am also working on is to support progress per request so that the client can upfront decide where to best render the UI.

[dbaeumer]

I published next versions of the VS Code LSP protocol, client and server libs that have support for progress.

[dbaeumer]

Please see also #786

[dbaeumer]

Closing the issue. Corresponding support has been speced in dbaeumer/3.15 branch.