Added restart java language server command

[AlexXuChen]

trafficstars

Added Restart Java Language Server command, java.language.server.restart.

Signed-off-by: Alexander Chen [email protected]

[AlexXuChen]

It seems the standard server won't start back up when the server mode is HYBRID

[rgrunber]

It seems the standard server won't start back up when the server mode is HYBRID

What happens if in the "Hybrid" case, you use syntaxClient.isAlive() and only stop/start if true. Otherwise, just stop/start the standard client for now.

[rgrunber]

StandardLanguageClient.start() does nothing because the underlying languageClient is null. It's null because StandardLanguageClient.stop() sets it that way (look inside the declaration).

Originally stop() was only really called on extension deactivate() so setting everything null was probably fine. Now, we want to stop the client but still re-use the connection so I would add a parameter to the stop(), something like boolean cleanup. It would be called with false for the restart command and true in deactivate()

[AlexXuChen]

Originally stop() was only really called on extension deactivate() so setting everything null was probably fine. Now, we want to stop the client but still re-use the connection so I would add a parameter to the stop(), something like boolean cleanup. It would be called with false for the restart command and true in deactivate()

Ah I see, because StandardLanguageClient.stop() is a wrapper for the LanguageClient.stop()

[rgrunber]

@testforstephen , you mentioned there's already an issue, or PR for this that's incoming ? I wasn't able to find any issue on the vscode-java-debug end.

[testforstephen]

@testforstephen , you mentioned there's already an issue, or PR for this that's incoming ? I wasn't able to find any issue on the vscode-java-debug end.

@rgrunber I tried this PR and Java debugger works fine after a language server restart. The reason is that the current restart implementation does not trigger a server mode change event and has no effect on the debugger.

Previously we saw some side effects when restarting because that implementation would destroy the languageClient and recreate a new one, which would cause the Java extension to resend the server mode change event.

However, I found another issue that the status icon is always spinning after language server restart.

[rgrunber]

I think I saw this as well initially. Our own wrappers destroy the entire client, but we don't need to do that, so I believe @AlexXuChen just made a separate method that just calls the necessary API on the LanguageClient.

Yes, I saw the spinning build status provider as well, and hopefully it can be fixed. As the startup process receives a lot of notifications and state changes, perhaps something isn't set correctly or expecting to go through an earlier phase again.

@testforstephen , if there's no PR planned (or other draft) on your end to address this, I think Alex should continue improving this, as it would be nice to have such a restart capability.

[datho7561]

I just tried this PR with vscode-microprofile, which provides a eclipse.jdt.ls extension and a language server that communicates with the eclipse.jdt.ls extension using language server commands. Restarting works very well; as soon as the server restarts, all the microprofile features start working again. The only issue I ran into while doing this is that the microprofile language server sends sends error notifications while eclipse.jdt.ls is down. This makes sense, since the microprofile language server is trying to make requests to eclipse.jdt.ls while it's not running. However, it might confuse the end user, since they might think something is wrong. It would be nice to prevent these errors from appearing, perhaps with an API to detect if the server is restarting, but I think that could wait to a future PR.

[rgrunber]

I'm still seeing the build status provider spinning even after the server has been restarted and is working normally. I think i had one or two instances where it didn't spin, but more often than not, it doesn't behave correctly. Looks like this has to do with whether the language server is able to send back the Shutdown... or progress report. Since the shutdown message is represented as incomplete, it results in the server status being set as busy, and since the project isn't re-imported, the last state will always be busy. One workaround we could do is to make sure the restart command sets the server status as ready after the restart is complete :

diff --git a/src/extension.ts b/src/extension.ts
index d76364c..c999912 100644
--- a/src/extension.ts
+++ b/src/extension.ts
@@ -1074,6 +1074,7 @@ function registerRestartJavaLanguageServerCommand(context: ExtensionContext) {
                        case (ServerMode.STANDARD):
                                // Standard server restart
                                await standardClient.restart();
+                               serverStatusBarProvider.setReady();
                                break;
                        case (ServerMode.LIGHTWEIGHT):
                                // Syntax server restart
@@ -1087,6 +1088,7 @@ function registerRestartJavaLanguageServerCommand(context: ExtensionContext) {
                                        await new Promise((resolve) => setTimeout(resolve, 3000));
                                }
                                await standardClient.restart();
+                               serverStatusBarProvider.setReady();
                                break;
                }
        }));

[AlexXuChen]

Server status should no longer be busy.

[testforstephen]

The message id doesn't exist in package.nls.json.

let me know if this works for you now. I think the last thing to test is specifically how it interacts when one is using vscode-java-debug or anything else.

Tried again with the latest commit and if there is a debug session running, the restart operation will terminate the debugger. Once the server is ready, the debugging feature works again. It doesn't break the Java debugger, which is good for me.

Just one concern is about the transparency of the operation. When I execute "Restart Java Language Server", there is currently no indicator to tell me what is happening in the background and when the server will be ready again. I'm wondering whether it's better to show a progress notification on restart opreation.

[rgrunber]

The only other issue I see is that in syntax client mode ("java.server.launchMode": "LightWeight"), I get a popup indicating the command to restart resulted in an error, "Connection is disposed". The server does not restart. If I call the same command while the language server is down, it does start back up.

Update : This appears to be https://github.com/redhat-developer/vscode-java/issues/2366

Just one concern is about the transparency of the operation. When I execute "Restart Java Language Server", there is currently no indicator to tell me what is happening in the background and when the server will be ready again. I'm wondering whether it's better to show a progress notification on restart opreation.

This could probably be done with window.withProgress(..), (which we use in a few places). It would at least indicate how long the restart operation takes but to have the progress disappear exactly when the server is back up (rather when the restart operation finishes) might be tricky. Maybe listening for any notification after the restart would be indication it has returned and then disposing of that onNotification listener might work.

[rgrunber]

In the process of trying to make the progress notification disappear once the server is responding again (as opposed to once the restart command has finished being issued), I've discovered that once the language server is restarted, it fails to receive notifications on any of the onNotification(...) callbacks. That's... not good.

Although this is a deal breaker, it's actually https://github.com/microsoft/vscode-languageserver-node/issues/770, which was fixed in 8.0. So we now depend on https://github.com/redhat-developer/vscode-java/issues/2474 in order to merge this change. Hopefully we'll be able to do that pretty soon.

[JessicaJHee]

As a side note, I run into the following problem while in debug mode sometimes:

ERROR: transport error 202: bind failed: Address already in use
ERROR: JDWP Transport dt_socket failed to initialize, TRANSPORT_INIT(510)
JDWP exit error AGENT_ERROR_TRANSPORT_INIT(197): No transports initialized [../src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:734]

The problem is solved with the timeout set to 2000 instead of 1000 here: https://github.com/microsoft/vscode-languageserver-node/blob/ffaf05ddd1fa9ca5f258cd0b08cc6cd1794c5b57/client/src/node/main.ts#L197

[rgrunber]

In the previous iteration of this PR, we had it set at 2000 (I suspect I saw issues with 1000). However now it's handled by the library so we don't really have control. It's mainly an annoying problem for developers because in production those debug ports aren't open (so no cases of LS shutdown still bound to 1044 while new one starting attempts to bind to 1044) though.

Worst case, we could just do what we did before. Copy the restart() logic and do it ourselves (replacing 1000 -> 2000). I'll try and see how bad it is.

[rgrunber]

This works really well for me with the eclipse.jdt.ls modification to the Shutdown job. I've tried restarting with various launch modes. I noticed that if no server is running (eg. invisible project with no files opened, or empty workspace), one can still restart the language server (though it never started once to begin with). It doesn't appear harmful. Still seems to succeed although maybe we shouldn't allow this to happen. Have a look at the "when": "javaLSReady" context for commands in the palette and let me know if that makes sense for such cases.