No need to run didOpen/didClose/didChange with scheduling rule

[testforstephen]

As discussed in #2518 and redhat-developer/vscode-java#3077 (comment), using scheduling rules in document lifecycle handlers can lead to deadlock and make the language server unresponsive during the build job. I propose to remove the scheduling rules from didOpen/didClose/didChange handlers for these reasons:

• didOpen/didClose handlers are some trivial tasks, and used to open or discard a workingcopy. It should be fine not to run it with scheduling rule.
• didChange handler has two tasks: syncing the buffer with the text changes, and reconciling the Java Model with the buffer. The second reconcile/validation task was executed in a workspace job and already had a scheduling rule, so the question is whether the first one also needs one. According to the Eclipse implementation, it updates the document buffer without any scheduling rule when the user types code. You can debug the Eclipse behavior by adding a breakpoint on org.eclipse.core.internal.filebuffers.SynchronizableDocument.replace() and then typing code in Eclipse. So I think it should be ok for jdt.ls to follow the same approach on buffer synchronization of didChange handler.
• The original PR Performance issue on large project #1479 that added the scheduling rules to didOpen/didClose/didChange was for performance improvement. That means the language server had no functional issues before the scheduling rules were introduced. Since they have more negative impact on performance, it’s worth trying to remove them.

[jdneo]

I think it worth to give it a shot in the pre-release channel first. And keep an eye on two targets:

• performance change
• exception rate

[testforstephen]

One more thing I forgot to mention is that using a scheduling rule in the didChange handler also has a negative impact on the debounce mechanism of the validation/diagnostics job. This is because the running validation/diagnostics job will lock the resource, and the next didChange will fail to acquire the lock and will not be able to cancel the previous validation/diagnostics job.

[testforstephen]

I think it worth to give it a shot in the pre-release channel first. And keep an eye on two targets:

• performance change
• exception rate

Good point. We have monitored the uncaught exceptions from language client level, we can use that to verify the reliability status after this PR is merged.

[rgrunber]

Could we got a step further (assuming no major issues) and not run the jobs in a workspace runnable, much like we attempted in #2449 ?

Is there any greater risk of text changes being applied out of order when typing quickly (given that each didChange is a separate thread coming as a request to the LS ?

[testforstephen]

Could we got a step further (assuming no major issues) and not run the jobs in a workspace runnable, much like we attempted in #2449 ?

Agree. Most document lifecycle operations do not modify the resource tree and do not require a workspace runnable. Only some rare cases need special handling. For instance, when an external Java file is opened, JLS will link it to the default project. We can handle such cases in a workspace runnable only.

Is there any greater risk of text changes being applied out of order when typing quickly (given that each didChange is a separate thread coming as a request to the LS ?

There should be no problem. The document buffer update is done synchronously in the jsonrpc thread, and the changes are applied in the order they arrive. This is because ResourcesPlugin.getWorkspace().run(new IWorkspaceRunnable()…) runs in the current thread, not a new one.

You can see more information about Jobs API on https://www.eclipse.org/articles/Article-Concurrency/jobs-api.html.