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

Automatically rename file to type name on save #3091

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

hopehadfield
Copy link
Contributor

@hopehadfield hopehadfield commented Mar 6, 2024

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't run this yet, but seems like it should work. I'm only wondering if there's a way to really simplify this.

@hopehadfield hopehadfield force-pushed the 3408-rename branch 2 times, most recently from a1fdcc3 to 428f537 Compare March 8, 2024 21:58
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine so far. just some small things.

edit.setChanges(Collections.emptyMap());
final boolean applyNow = JavaLanguageServerPlugin.getPreferencesManager().getClientPreferences().isWorkspaceApplyEditSupported();
if (applyNow) {
JavaLanguageServerPlugin.getInstance().getClientConnection().applyWorkspaceEdit(edit);
Copy link
Contributor

@rgrunber rgrunber Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our discussion, this call, during willSaveWaitUntil short circuits the client-side saving logic (you're effectively deleting a file that is in the process of being saved) and the result is didSave never occurs, which causes improper saving (and a dirty editor). Maybe moving the logic into didSave itself is a better approach.

@rgrunber
Copy link
Contributor

rgrunber commented Mar 15, 2024

Just for the sake of documenting. We ran into an issue here where the language server completely stopped responding to requests after workspace/applyEdit call that went from client -> server. The Java process was still up, and running. Just no handling of incoming messages, despite the client clearly indicating they were sent.

The main hint came when I decided to track which thread was handling the workspace/applyEdit, and observed :

Screenshot from 2024-03-14 16-03-19

So the listening thread handled the incoming didSave, and send a workspace/applyEdit, while waiting for the client to respond with whether it was successful. The client responded, but no response/request could be processed because the server was busy waiting for it in the listening thread 😐 .

Turns out this is documented at https://github.com/eclipse-lsp4j/lsp4j/blob/main/documentation/README.md#launch-and-connect-with-a-languageclient and mentioned at eclipse-lsp4j/lsp4j#775 :

When implementing the handlers for requests or notifications, you need to be aware that the calling thread is the thread that reads and dispatches incoming messages. Therefore, blocking it may result in reduced throughput or even a deadlock (eclipse-lsp4j/lsp4j#775). As a general rule, message handlers should be implemented in a non-blocking, asynchronous way.

This should definitely serve as a warning because we have a lot of helper methods in JavaClientConnection where we call an underlying server request (to the client), and then join() on it in the same thread that the listening is happening. If any of those calls require the server to respond, it will trigger a deadlock because the thread will not be able to read the response while waiting for it. In a lot of instances it's not an issue because the underlying helper call is done in a spawned thread.

@hopehadfield hopehadfield force-pushed the 3408-rename branch 2 times, most recently from 7f250b1 to d8c9e62 Compare March 15, 2024 20:48
@rgrunber
Copy link
Contributor

If you cause a rename and try to perform some actions in between, it can cause these stacktraces in some instances :

!MESSAGE Failed to check constructor status
!STACK 1
Java Model Exception: Core Exception [code 368] File not found: /home/rgrunber/git/lemminx/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLLanguageServerAST.java.
	at org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray(Util.java:1206)
	at org.eclipse.jdt.internal.core.util.Util.getResourceContentsAsCharArray(Util.java:1196)
	at org.eclipse.jdt.internal.core.CompilationUnit.openBuffer(CompilationUnit.java:1210)
	at org.eclipse.jdt.internal.core.CompilationUnit.buildStructure(CompilationUnit.java:107)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:245)
	at org.eclipse.jdt.internal.core.SourceRefElement.generateInfos(SourceRefElement.java:128)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:585)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:308)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:294)
	at org.eclipse.jdt.internal.core.Member.getNameRange(Member.java:366)
	at org.eclipse.jdt.internal.corext.dom.ASTNodes.getTypeBinding(ASTNodes.java:3887)
	at org.eclipse.jdt.ls.core.internal.handlers.GenerateConstructorsHandler.checkConstructorStatus(GenerateConstructorsHandler.java:89)
	at org.eclipse.jdt.ls.core.internal.text.correction.SourceAssistProcessor.getGenerateConstructorsAction(SourceAssistProcessor.java:517)
	at org.eclipse.jdt.ls.core.internal.text.correction.SourceAssistProcessor.getSourceActionCommands(SourceAssistProcessor.java:132)
	at org.eclipse.jdt.ls.core.internal.handlers.CodeActionHandler.getCodeActionCommands(CodeActionHandler.java:223)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$14(JDTLanguageServer.java:766)
	at org.eclipse.jdt.ls.core.internal.BaseJDTLanguageServer.lambda$0(BaseJDTLanguageServer.java:87)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)

I've seen it for codelens, hover, and folding range. The failure is basically in getBuffer() or getNameRange(). I wonder if we can just handle these like we did with the previous example.

@rgrunber rgrunber merged commit 661b7a4 into eclipse-jdtls:master Mar 18, 2024
7 checks passed
@rgrunber rgrunber added this to the End March 2024 milestone Apr 1, 2024
@hopehadfield hopehadfield deleted the 3408-rename branch April 23, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants