Integrate rust-analyzer and improve LSP Integration (required function, guards for empty replies, icons, deadlock fix)#9249
Conversation
…hDiagnostic Test with rust-analyzer yielded the following exception: java.lang.UnsupportedOperationException at org.eclipse.lsp4j.services.LanguageClient.refreshDiagnostics(LanguageClient.java:251) Caused: java.lang.reflect.InvocationTargetException at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$null$0(GenericEndpoint.java:65) Caused: java.lang.RuntimeException at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$null$0(GenericEndpoint.java:67) at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.request(GenericEndpoint.java:120) [catch] at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleRequest(RemoteEndpoint.java:261) at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.consume(RemoteEndpoint.java:190) at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.handleMessage(StreamMessageProducer.java:194) at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:94) at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:113) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) at java.base/java.lang.Thread.run(Thread.java:840)
…cument It was observed, that with the rust-analyzer language server inserting a completion led to a dead-lock. Checking the stacks of running threads shows, that the LS tries to publish diagostics, while in parallel a documentFormat is called. The assumption is, that the rust side block execution of the `documentFormat` request until the `publishDiagnostics` call from the LS finishes. The latter though needs access to the document lock and that is already taken by the `documentFormat` task. "AWT-EventQueue-0" apache#35 prio=6 os_prio=0 cpu=8224,39ms elapsed=119,39s tid=0x00007f5384156f60 nid=0x2b0da7 waiting on condition [0x00007f537e7f9000] java.lang.Thread.State: WAITING (parking) at jdk.internal.misc.Unsafe.park(java.base@17.0.9/Native Method) - parking to wait for <0x00000006448493e8> (a java.util.concurrent.CompletableFuture$Signaller) at java.util.concurrent.locks.LockSupport.park(java.base@17.0.9/LockSupport.java:211) at java.util.concurrent.CompletableFuture$Signaller.block(java.base@17.0.9/CompletableFuture.java:1864) at java.util.concurrent.ForkJoinPool.unmanagedBlock(java.base@17.0.9/ForkJoinPool.java:3465) at java.util.concurrent.ForkJoinPool.managedBlock(java.base@17.0.9/ForkJoinPool.java:3436) at java.util.concurrent.CompletableFuture.waitingGet(java.base@17.0.9/CompletableFuture.java:1898) at java.util.concurrent.CompletableFuture.get(java.base@17.0.9/CompletableFuture.java:2072) at org.netbeans.modules.lsp.client.bindings.Formatter.documentFormat(Formatter.java:122) at org.netbeans.modules.lsp.client.bindings.Formatter.reformat(Formatter.java:87) at org.netbeans.modules.editor.indent.TaskHandler$MimeItem.runTask(TaskHandler.java:550) at org.netbeans.modules.editor.indent.TaskHandler.runTasks(TaskHandler.java:309) at org.netbeans.modules.editor.indent.IndentImpl.reformat(IndentImpl.java:349) at org.netbeans.modules.editor.indent.api.Reformat.reformat(Reformat.java:129) at org.netbeans.lib.editor.codetemplates.CodeTemplateInsertHandler.run(CodeTemplateInsertHandler.java:352) at org.netbeans.editor.GuardedDocument.runAtomicAsUser(GuardedDocument.java:333) at org.netbeans.lib.editor.codetemplates.CodeTemplateInsertHandler.insertTemplate(CodeTemplateInsertHandler.java:258) at org.netbeans.lib.editor.codetemplates.CodeTemplateInsertHandler.processTemplate(CodeTemplateInsertHandler.java:229) at org.netbeans.lib.editor.codetemplates.CodeTemplateManagerOperation.insert(CodeTemplateManagerOperation.java:273) at org.netbeans.lib.editor.codetemplates.api.CodeTemplate.insert(CodeTemplate.java:82) at org.netbeans.modules.lsp.client.bindings.CompletionProviderImpl$3.lambda$commit$0(CompletionProviderImpl.java:307) at org.netbeans.modules.lsp.client.bindings.CompletionProviderImpl$3$$Lambda$825/0x00007f53e4ae8498.run(Unknown Source) at org.netbeans.editor.GuardedDocument.runAtomic(GuardedDocument.java:296) at org.openide.text.NbDocument.runAtomic(NbDocument.java:411) at org.netbeans.modules.lsp.client.bindings.CompletionProviderImpl$3.commit(CompletionProviderImpl.java:254) at org.netbeans.modules.lsp.client.bindings.CompletionProviderImpl$3.defaultAction(CompletionProviderImpl.java:231) at org.netbeans.modules.editor.completion.CompletionImpl.dispatchKeyEvent(CompletionImpl.java:785) at org.netbeans.modules.editor.completion.CompletionImpl.keyPressed(CompletionImpl.java:386) at java.awt.AWTEventMulticaster.keyPressed(java.desktop@17.0.9/AWTEventMulticaster.java:258) [...] "pool-4-thread-1" apache#75 prio=5 os_prio=0 cpu=136,37ms elapsed=76,96s tid=0x00007f5380ae1080 nid=0x2b0e84 in Object.wait() [0x00007f52e99fc000] java.lang.Thread.State: WAITING (on object monitor) at java.lang.Object.wait(java.base@17.0.9/Native Method) - waiting on <no object reference available> at java.lang.Object.wait(java.base@17.0.9/Object.java:338) at javax.swing.text.AbstractDocument.readLock(java.desktop@17.0.9/AbstractDocument.java:1421) - locked <0x000000063c6af680> (a org.netbeans.modules.csl.core.GsfDocument) at org.netbeans.editor.BaseDocument.render(BaseDocument.java:1405) at org.openide.text.PositionRef$Manager$DocumentRenderer.render(PositionRef.java:927) at org.openide.text.PositionRef$Manager$DocumentRenderer.renderToObject(PositionRef.java:948) at org.openide.text.PositionRef$Manager.addPosition(PositionRef.java:389) at org.openide.text.PositionRef.init(PositionRef.java:105) at org.openide.text.PositionRef.<init>(PositionRef.java:90) at org.openide.text.PositionRef.<init>(PositionRef.java:68) at org.openide.text.CloneableEditorSupport.createPositionRef(CloneableEditorSupport.java:1264) at org.netbeans.modules.editor.hints.HintsControllerImpl.linePart(HintsControllerImpl.java:231) at org.netbeans.spi.editor.hints.ErrorDescriptionFactory.createErrorDescription(ErrorDescriptionFactory.java:245) at org.netbeans.spi.editor.hints.ErrorDescriptionFactory.createErrorDescription(ErrorDescriptionFactory.java:218) at org.netbeans.modules.lsp.client.bindings.LanguageClientImpl.lambda$publishDiagnostics$0(LanguageClientImpl.java:176) at org.netbeans.modules.lsp.client.bindings.LanguageClientImpl$$Lambda$705/0x00007f53e4ac73a8.apply(Unknown Source) at java.util.stream.ReferencePipeline$3$1.accept(java.base@17.0.9/ReferencePipeline.java:197) at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(java.base@17.0.9/ArrayList.java:1625) at java.util.stream.AbstractPipeline.copyInto(java.base@17.0.9/AbstractPipeline.java:509) at java.util.stream.AbstractPipeline.wrapAndCopyInto(java.base@17.0.9/AbstractPipeline.java:499) at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(java.base@17.0.9/ReduceOps.java:921) at java.util.stream.AbstractPipeline.evaluate(java.base@17.0.9/AbstractPipeline.java:234) at java.util.stream.ReferencePipeline.collect(java.base@17.0.9/ReferencePipeline.java:682) at org.netbeans.modules.lsp.client.bindings.LanguageClientImpl.publishDiagnostics(LanguageClientImpl.java:177) at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@17.0.9/Native Method) at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@17.0.9/NativeMethodAccessorImpl.java:77) at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@17.0.9/DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(java.base@17.0.9/Method.java:568) at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$recursiveFindRpcMethods$0(GenericEndpoint.java:65) at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint$$Lambda$648/0x00007f53e4a4dd50.apply(Unknown Source) at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.notify(GenericEndpoint.java:160) at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleNotification(RemoteEndpoint.java:231) [...]
- add support for rust LSP implementation rust-analyzer - align rust icons more with existing NB icons - show hint if rust analyser is not configured
|
Hi @matthiasblaesing , Thanks for this effort! I wasn't able to get LSP server running properly with: The NetBeans UI froze, with the EDT trying to update nodes [1]. I haven't found time to look at it in detail, but from [2] I wonder if we should be using I think it's premature to add this to the IDE right now. Maybe we want to iterate a little bit more. Kind regards, [1] [2] |
|
The thread dump.txt I was able to extract when the IDE froze. |
rust/rust.grammar/src/org/netbeans/modules/rust/grammar/lsp/RustLSP.java
Show resolved
Hide resolved
rust/rust.grammar/src/org/netbeans/modules/rust/grammar/lsp/RustLSP.java
Show resolved
Hide resolved
I'll look into the points you brought up. BUT I have to ask: for what do we want to wait? Rust Support in NetBeans has not moved an inch and while this is rough, not doing it will not yield improvements. |
|
Where does that version of rust-analyzer come from? I looked through https://github.com/rust-lang/rust-analyzer/releases and https://github.com/rust-lang/rust-analyzer/tags there is neither a tag, nor a release entry in september of last year. I fail to find the reference |
|
@vieiro could you please describe how you reached the deadlock and whether that is reproducible? |
Very true! If this is on master we'll have more chances to get contributions! |
I had a project opened (and a Rust file opened in the editor). Then the IDE got restarted (after changing the rust-analyzer path) and then it happened. I think there's a race condition between the LSP server being started and the opened files being sent to the LSP server, or something like that. IIRC there were two rust-analyzer servers running at the same time. I'll try to get a reproducer, though. |
| updateFocused(); | ||
| } | ||
| }); | ||
| updateFocused(); |
There was a problem hiding this comment.
just a guess regarding the reported freeze:
- wrap this line into SwingUtilities.invokeLater(line) or replace
@OnStartwith@OnShowing - remove synchronized from
updateFocused()(since no longer needed)
There was a problem hiding this comment.
It could be.
Maybe we should redesign the LSPClient in NetBeans.
This should be a state machine, that queues requests in order and does not execute them until the LSP server has been properly initialized.
I have a small draft that initializes servers somewhere (but all the rest is still missing). May want to recover it from backup...
There was a problem hiding this comment.
wondering if the updateFocused() call from the run method is even needed. This appears as if the worst case would be that you would only see the hint after you focus the editor - which doesn't sound that bad.
There was a problem hiding this comment.
another thought (still without having tried it - i will later I promise). Listening for property changes tends to scale badly. They can be called over a million times on startup or project group changes which can become a bottle neck (#8955).
I am not sure if it applies here too but would be worth to see how often this is called and consider filtering for concrete event keys and adding fast paths to the method which exit early.
There was a problem hiding this comment.
This is established code (as in: This code was more or less copied from cpplite).
Question: Where does the idea come from, that this code has anything to do with the hang? There is nothing in the dump that indicates this to me. If it would be the synchronized call, we would need to see a call into UnconfiguredHint in the stacktraces, which is not the case.
For the rewrite of LSP support: A good rewrite might improve the situation, but experience shows, that rewrites also bring up new bugs. What we see with the rust lsp is that "the other" side is now also multithreaded. The primary use-case of LSP support in NetBeans was the typescript lsp and given javascripts inherent single threaded nature we are less likely to get into deadlock situations.
The crucial point: The problem fixed here (see d549c44) that was reproducible, if this is not, I don't see the discussion as productive.
There was a problem hiding this comment.
This code was more or less copied from cpplite).
I expect ccplite having very low usage given that the project import on NB version migration doesn't even work atm. And users asking on the discussions how to set projects up with no answers.
Question: Where does the idea come from, that this code has anything to do with the hang?
I looked through the diff focusing on concurrency red flags. Calling from EDT into a synchonized block is one. It being a startup/init issue while some prop change listeners are under high load during startup is another.
if this is not, I don't see the discussion as productive.
could still be a problem. But if its not appreciated I'll stop I have other things to do anyway ;)
There was a problem hiding this comment.
Sorry, that came over more negative that it should have sounded.
At this point in time there is one lockup reported and it is unclear whether it is related. We have sychronized code and I agree, that it could be a problem, but then no connection between that and the observed problem.
For the observed problem the EDT is not not blocked (Mutex#isReadAccess is method that calls through into the real implementation and that would need to show up). The callstack looks deep, but the LSP code does not seem to do problematic things (the document structure is extracted en-block and children creation is just creating java object).
| @OnStart | ||
| public class UnconfiguredHint implements Runnable { | ||
|
|
||
| private static final Set<String> RUST_MIME_TYPES = new HashSet<>(Arrays.asList("text/x-rust")); |
There was a problem hiding this comment.
would be also a good opportunity to switch the rust modules to javac.release=17 since I noticed that there weren't migrated yet. (#8813)
|
Hi @matthiasblaesing , I was trying today again with different projects and couldn't reproduce the IDE freeze. I've started the rust-analyzer with these options to have more details on what's going on. No need to include these in the commit, I'm posting them here just in case they're of interest in the future. diff --git a/rust/rust.grammar/src/org/netbeans/modules/rust/grammar/lsp/RustLSP.java b/rust/rust.grammar/src/org/netbeans/modules/rust/grammar/lsp/RustLSP.java
index 0a8bdda40c..c1b2aee366 100644
--- a/rust/rust.grammar/src/org/netbeans/modules/rust/grammar/lsp/RustLSP.java
+++ b/rust/rust.grammar/src/org/netbeans/modules/rust/grammar/lsp/RustLSP.java
@@ -39,16 +39,23 @@ public class RustLSP implements LanguageServerProvider {
public LanguageServerDescription startServer(Lookup lookup) {
Path rustAnalyzerPath = RustAnalyzerOptions.getRustAnalyzerLocation(true, true);
if(rustAnalyzerPath == null || ! Files.isExecutable(rustAnalyzerPath)) {
+ LOG.log(Level.INFO, String.format("rustAnalyzerPath is not available: %s", rustAnalyzerPath));
return null;
}
try {
- Process p = new ProcessBuilder(new String[]{rustAnalyzerPath.toAbsolutePath().toString()})
+ Process p = new ProcessBuilder(new String[]{
+ rustAnalyzerPath.toAbsolutePath().toString(),
+ "--verbose",
+ "--log-file",
+ "/tmp/rust-analyzer.log",
+ "--no-log-buffering"
+ })
.directory(rustAnalyzerPath.getParent().toFile())
.redirectError(ProcessBuilder.Redirect.INHERIT)
.start();
return LanguageServerDescription.create(p.getInputStream(), p.getOutputStream(), p);
} catch (IOException ex) {
- LOG.log(Level.FINE, null, ex);
+ LOG.log(Level.INFO, String.format("Error launching rust-analyzer: %s", ex.getMessage()), ex);
return null;
}
}I found some other issues, though, but I think we can work out these in the future. I'll post them here for future reference: And Thanks for the PR. I'm approving changes now! |
vieiro
left a comment
There was a problem hiding this comment.
Good to go. Let's get this merged-in, and improve in the future.
|
FWIW, having the For instance, if you mouse+click on a Rust The process keeps on running well but who knows, maybe it causes a GUI freeze in other circumstances... |
General improvements (lsp.client)
To be able to sanely use rust-analyzer (the rust LSP implementation) the NetBeans lsp client implementation needs some adjustments:
refreshDiagnostics, failing to correctly work if that is not the casenullas a result of LSP operations. The existingnullguards were thus extendedpublishDiagnosticscall as part of the formatting.Rust specific
The cargo TOML support is updated to the current version of TOML to be able to parse project files using extended syntax.
While NetBeans can provide parts of the IDE services based on custom implementations, the rust-analyzer LSP server provides extended support of the box. Compared with the built-in solution rust-analyzer has the drawback, that it is huge and platform dependent, so it can't be bundled with NetBeans.
To provide both minimal rust support in all cases and improved editing support by using rust-analyzer the solution provided here will switch from internal support to rust-analyzer only if rust-analyzer is installed and configured.
When a user now opens a rust file and rust-analyzer is not configured a warning is attached to the file (as is done for cpplite):
But structure is still present:
The icons used in the rust module were aligned with the other NetBeans icons from lsp.client and PHP support.
The warning allows to directly jump into options to configure rust-analyzer:
With that done extended structure support is activated:
Code completion is provided:
and Find usages works: