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

Progress support in LSP #786

Closed
dbaeumer opened this issue Jul 3, 2019 · 5 comments
Closed

Progress support in LSP #786

dbaeumer opened this issue Jul 3, 2019 · 5 comments
Labels
feature-request Request for new features or functionality
Milestone

Comments

@dbaeumer
Copy link
Member

dbaeumer commented Jul 3, 2019

There are two distinct requests in LSP that ask for progress:

  • one to report work done progress on requests so that clients can show a progress monitor
  • one to report partial results on requests that are long running. For example workspace symbols or find all references.

The corresponding items are:

#70
#110
#182

There has been a long discussion in microsoft/vs-streamjsonrpc#139 on how to best do this in a homogeneous why in JSON-RPC and I propose that we implement the outcome of this issue in LSP as well.

The basic idea is as follows:

  • clients indicate that they accept progress notifications from the server by adding a token property to the request params
  • server use the $/progress notification and the provided token to report progress.
{
  "jsonrpc": "2.0",
  "method": "$/progress",
  "params": {
    "token": "the token provided in the request",
    "value": { "some": "value" }
  }
}

LSP should standardize the token property names used to indicate the progress types:

  • workDone: to indicate that the client accepts work done progress.
  • partialResult: to indicate that the client accepts partial result for a request.

Work Done

The value of the work done progress notification would be the same as for the window/progress/* notifications which support progress initiated from the server.

If the server supports progress it is supposed to send a first notification indicating the start.

export interface ProgressStart {

	kind: 'start';

	/**
	 * 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;
}

followed by many progress report messages:

export interface ProgressReport {

	kind: 'report';


	/**
	 * 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;
}

an an optional done(end is implicit when the request sends a response):

export interface ProgressDoneParams {
    kind: 'done';
}

I would also adjust the proposed server initiated progress accordingly (e.g. having one one notification route with and an additional kind property). See https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.progress.proposed.ts#L1

Partial Result

The value property of the partial result depends on the kind of request and needs to be specified on a request to request basis.

For example for the workspace/symbol request the value of a partial progress notification would be SymbolInformation[].

If a client accepts partial result for a request and the server sends partial result the final response must not carry any additional result data. The value should either be null or an empty data structure (e.g. empty array). The final response can of course still contain error information.

@dbaeumer dbaeumer added this to the 3.15 milestone Jul 3, 2019
@dbaeumer dbaeumer added the feature-request Request for new features or functionality label Jul 3, 2019
@olegtk
Copy link
Member

olegtk commented Jul 3, 2019

this looks awesome! couple comments:

  1. In Visual Studio there are some long running operation scenarios when Cancel button needs to be disabled dynamically as operation progresses and gets to the point of no cancellation (when side effects were incurred and cancelling would leave system in an inconsistent state). Would it be reasonable to include cancellable property to the ProgressReport too?
  2. Also in some scenarios server can only provide incomplete results because of system state (e.g. when full result set is not yet available due to indexing) or there are too many results. It would be great if ProgressDoneParams allowed for servers to indicate such case and provide a reason why results are incomplete.

@tinaschrepfer
Copy link
Member

Per our discussion regarding when errors occur during streaming progress, the decision is to keep current set of results reported thus far, but report an error/warning notification to user indicating something went wrong and results may be incomplete.

@sean-mcmanus
Copy link

sean-mcmanus commented Jul 19, 2019

Our C/C++ extension Find All References sends back references with types like "Possible Reference (confirmation in progress)" and we later update it to "Not a Reference", "Confirmed Reference", "Possible Reference (confirmation cancelled)", "Cannot confirm reference, etc.-- the API described here doesn't appear to allow partial results that overwrite all the existing results instead of just results that are accumulated. Could you add a progress support that supports this, such as via an "overwriteResult" progress type? We're (currently) okay with overwriting all the results and not just a subset of results that are potentially changeable and overwritable with better results later, but it's possible there could be future scenarios where this is not good, if there are too many results to overwrite -- I guess the "partialResult" could be used for stuff that is not overwritable and the "overwriteResult" could form at different set. There is another LSP request for "typed references" to match VS.

We currently implement our own "streaming" via causing a 2nd textDocument/references request that overwrites the previous results (when we want to update the results), but there are bugs, because this type of "hack" wasn't intentionally designed for, e.g. it breaks if the cursor is moved off an identifier (because it canceled out too early without sending us the LSP message), and also any popup menus get closed when the final results come in (caused by executing the references-view.find command).

@sean-mcmanus
Copy link

Ah, the details of a streaming textDocument/references were already discussed in https://github.com/microsoft/ms-lsp/issues/10 .

@dbaeumer
Copy link
Member Author

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

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

4 participants