Fix document corruption due to delayed evaluation in protocolFilter

[Colengms]

This should address #10035 as well as some document corruption issues related to CDD.

The issue arose in protocolFilter.ts due to use of deferred lambdas. That caused some TypeScript objects to be re-evaluated later, when the message was actually sent, instead of within the protocolFilter handler at the time the message was intended to be processed. For example, when processing a didChange message, the TextDocument has a particular document version number, but we would get a more recent version number when sendMessage was actually called after deferred through notifyWhenLanguageClientReady. The mismatching version numbers resulted in incorrect management of the contents of the buffer cache in the native process.

It doesn't seem necessary to defer using notifyWhenLanguageClientReady, as the protocolFilter would not be processing messages if it were not ready to.

I also removed the call to processDelayedDidOpen from didChange, as it could attempt to defer as well. This was a failsafe anyway. We should not be getting calls to didChange without having previously processed a didOpen (delayed or otherwise). If we did, that would seem to indicate a more serious issue elsewhere.

Alternatively, this perhaps could have been addressed using await with the protocol handler, as those can now be async. However, I expect that could be vulnerable to deadlock. For example: If it were to try to wait for message B before allowing delivery of message A to complete, that would deadlock the queue. Instead, the logic within protocolFilter should be careful to ensure that sendMessage occurs in a synchronous manner before returning.

[bobbrow]

It doesn't seem necessary to defer using notifyWhenLanguageClientReady, as the protocolFilter would not be processing messages if it were not ready to.

notifyWhenLanguageClientReady has another purpose which is to serialize the messages so that the order is preserved. If the problem is that the document changes between calls, then I would think saving the old values would be the safer option.

[bobbrow]

In other words, I'm concerned that removing the calls to notifyWhenLanguageClientReady will cause different ordering problems with the messages.

[Colengms]

In other words, I'm concerned that removing the calls to notifyWhenLanguageClientReady will cause different ordering problems with the messages.

The protocol filter is invoked for each message in the queue, giving us an opportunity to make a change to the message or augment what happens when that message is processed. By using notifyWhenLanguageClientReady (and not awaiting), we were actually taking certain messages out of the queue, allowing other messages to be processed, then later delivering the message.

The bug here was specifically due to how deferring sendMessage with notifyWhenLanguageClientReady resulted in parameters to sendMessage being evaluated later. Specifically, the TextDocumentChangeEvent contains a TextDocument, and we were seeing that the value of version represented the current documentation version at the (later) time that sendMessage was invoked, rather than at the time the message was intended to be processed, so no longer matched the edit described by that TextDocumentChangeEvent. I assume that would be because VS Code is still actively using and modifying that TextDocument instance, rather than creating a copy for use in TextDocumentChangeEvent (which could also have addressed the issue).

Removing notifyWhenLanguageClientReady in order to retain the original order in which the messages were in the queue, and ensuring the parameters to sendMessage are evaluated when intended, was the fix.

[bobbrow]

I'm trying to reconcile this with the code that was retained. All of the other messages still use the notify queue, so it seems that the actual issue is that we need to clone the object, not that we need to remove the notify call.

    const defaultHandler: (data: any, callback: (data: any) => Promise<void>) => Promise<void> = async (data, callback: (data: any) => void) => { clients.ActiveClient.notifyWhenLanguageClientReady(() => callback(data)); };
    // const invoke1 = async (a: any, next: (a: any) => any) => { await clients.ActiveClient.awaitUntilLanguageClientReady(); return next(a); };
    const invoke2 = async (a: any, b: any, next: (a: any, b: any) => any) => { await clients.ActiveClient.awaitUntilLanguageClientReady(); return next(a, b); };
    const invoke3 = async (a: any, b: any, c: any, next: (a: any, b: any, c: any) => any) => { await clients.ActiveClient.awaitUntilLanguageClientReady(); return next(a, b, c); };
    const invoke4 = async (a: any, b: any, c: any, d: any, next: (a: any, b: any, c: any, d: any) => any) => { await clients.ActiveClient.awaitUntilLanguageClientReady(); return next(a, b, c, d); };

[Colengms]

I actually overlooked that potential deferral due to awaitUntilLanguageClientReady. I don't believe they are needed or desirable. protocolFilter should already be serialized. We know the language service is ready to process messages, as the protocolFilter is invoked as part of that pipeline. Because those calls are 'awaited', they would not cause messages to be processed out of order. I'm not sure whether they are vulnerable to the 'delayed evaluation' issue. Perhaps they are either always completing immediately or represent additional potential repros of the late evaluation bug. (We definitely saw that the root cause of the issue was that the version number was too recent in some didChange messages, and that issue was addressed by longer allowing sendMessage to be deferred (without awaiting it). Doing a deep copy of all message params seems a bit unbounded.

I don't believe anything we are doing within the protocol filter justifies delaying allowing those messages to continue to be processed immediately. Can you identify any specific requirement that protocolFilter delay processing a message until something in particular in the task queue has completed?

[bobbrow]

It has nothing to do with the language client being ready, but everything to do with the call to queueTask within the notifyWhenLanguageClientReady, awaitUntilLanguageClientReady, etc calls. All messages sent to the language server are serialized through this queue to retain their order. My fear is that removing the call opens the door to processing messages out of the intended order.

It didn't sound like you need to do a deep copy of all message params. Don't we just need to save the document version to check whether the message should be discarded or not?