Perform read lock on LSP requests #1640
Conversation
|
As what I understood from the discussion in the weekly dev meeting, the problem is the following: Maybe the solution in this PR is the only one, but I still want to share some other ideas (not sure it works well with Langium):
|
| */ | ||
| read<T>(action: () => MaybePromise<T>): Promise<T>; | ||
| read<T>(action: () => MaybePromise<T>, priority?: boolean): Promise<T>; |
There was a problem hiding this comment.
I am associating priority with an ordering/number. I understand the usage here as hasPriority. Maybe there is a better name, like: isUrgent, important, handleFirst...
There was a problem hiding this comment.
IMO a Boolean argument is problematic because seeing a method call like read(..., true) hides the semantics behind it. I'd use a string value like 'normal' | 'prioritized' instead.
| @@ -59,8 +64,25 @@ export class DefaultWorkspaceLock implements WorkspaceLock { | |||
| return this.enqueue(this.writeQueue, action, tokenSource.token); | |||
| } | |||
|
|
|||
| read<T>(action: () => MaybePromise<T>): Promise<T> { | |||
| return this.enqueue(this.readQueue, action); | |||
| read<T>(action: () => MaybePromise<T>, priority?: boolean): Promise<T> { | |||
There was a problem hiding this comment.
Just thinking loud: Is this the first time we have a "priority" queue?
You could refactor a common component/function if not.
There was a problem hiding this comment.
The workspace lock is a mutex, not a queue. It features some very specific semantics about read/read with prio/write actions that are likely not relevant for a common component.
There was a problem hiding this comment.
I share @dhuebner's concerns. Is it right to push through an LSP request even though the text has changed in the meantime?
Reading the specification section Implementation Considerations, it looks like the best solution would be to return an error code ContentModified when a change has happened.
| return await serviceCall(language, document, params, cancelToken); | ||
| const result = await lock.read(async () => { | ||
| return await serviceCall(language, document, params, cancelToken); | ||
| }, true); // Give this priority, since we already waited until the target state |
There was a problem hiding this comment.
Almost all LSP requests are now treated with priority? That seems too much to me: especially implicitly sent requests like DocumentHighlights should not block the build process. And what about Completion requests that are sent while typing?
There was a problem hiding this comment.
All of these are getting cancelled by the language client - they shouldn't be blocking the workspace in any way for longer than our timeout (5ms).
There was a problem hiding this comment.
Where does that 5 ms timeout come from?
| */ | ||
| read<T>(action: () => MaybePromise<T>): Promise<T>; | ||
| read<T>(action: () => MaybePromise<T>, priority?: boolean): Promise<T>; |
There was a problem hiding this comment.
IMO a Boolean argument is problematic because seeing a method call like read(..., true) hides the semantics behind it. I'd use a string value like 'normal' | 'prioritized' instead.
|
@spoenemann The link you've posted literally says this about changed content:
We do exactly this: Prevent the response from failing by blocking the workspace lock and returning the result. In the meantime, the client is free to cancel the pending request which we completely respect and unblock the workspace lock |
|
But that section also says:
This PR is about long running requests; when the state of a document is changed so that we can no longer finish the request, we should return an error. Note that this is not the same as canceling the request (which should be done only by the client). |
|
Could it help to use a utility function like this in LSP request processing? export async function interruptAndCheckDocument(document: LangiumDocument, token: CancellationToken): Promise<void> {
const previousState = document.state;
await interruptAndCheck(token);
if (document.state < previousState) {
throw new ResponseError(LSPErrorCodes.ContentModified, 'Document content has been modified.');
}
} |
The issue is: We cannot really know? For example, for the document highlight:
Aborting the initial operation for line 2 when receiving a document update is not the correct move here - the data for that highlighting is still valid and aborting the request might leave the user with incorrectly highlighted text. This is the case for most LSP requests. The internal state change the documentation is talking about (like project context switches) are much more fundamental changes than the "normal" document changes that make the result literally useless - however our results are probably still useful (except if the language client deems the change to be too large - in which case we get a cancellation anyway). |
There was a problem hiding this comment.
Yes, I see the problem with semantic highlighting (Semantic Tokens) (you said document highlighting, but that's a different service). The situation is different for other services. But it's true that it's the client that should decide whether to use a potentially outdated result or not.
We could apply this and then check how it affects the editing experience in larger projects. I'm particularly curious how it performs when completion is continuously triggered while typing.
| @@ -89,7 +111,7 @@ export class DefaultWorkspaceLock implements WorkspaceLock { | |||
| } else { | |||
| return; | |||
| } | |||
| this.done = false; | |||
| this.counter += entries.length; | |||
| await Promise.all(entries.map(async ({ action, deferred, cancellationToken }) => { | |||
| try { | |||
| // Move the execution of the action to the next event loop tick via `Promise.resolve()` | |||
There was a problem hiding this comment.
Unrelated to this change, but does awaiting Promise.resolve() really change the execution order in the event loop? Isn't that rather what setImmediate and our utility function delayNextTick are designed for?
We could rewrite this to:
await delayNextTick();
const result = await action(cancellationToken);| Promise.resolve(action()).then( | ||
| result => end(() => deferred.resolve(result)), | ||
| err => end(() => deferred.reject(err)) | ||
| ); |
There was a problem hiding this comment.
I see two potential issues with this code:
- The solution with the local
endfunction is a more complicated way of expressing afinallyblock. - I suspect that
Promise.resolve(action())does not behave as we want when theactionthrows an error – the error would just be propagated instead of rejecting the resulting promise.
I suggest making this whole method async so we can await the action and wrap that in a try-finally block.
| return await serviceCall(language, params, cancelToken); | ||
| const result = await lock.read(async () => { | ||
| return await serviceCall(language, params, cancelToken); | ||
| }, true); // Give this priority, since we already waited until the target state |
There was a problem hiding this comment.
I think this can be simplified to
await lock.read(() => serviceCall(language, params, cancelToken), true)(same in createServerRequestHandler and createRequestHandler).
|
Question: with this change, does it make any sense for LSP services to use Should we make it clear in the function documentation that it should be used only in the document building process and the associated services? |
I've noticed that some long running requests (i.e. completion in large files, sometimes semantic highlighting) can break pretty heavily and lead to unresolved references if the specified document gets a new build triggered. This change adds a new type of read to the
WorkspaceLockservice that allows to instantly serve a read request - all other requests are queued up until this read request has finished (or has been aborted).