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

java server refreshing the workspace for each restart on Windows #880

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Apr 12, 2019

Fixes #793

Signed-off-by: Snjezana Peco [email protected]

@@ -66,12 +67,6 @@ function prepareParams(requirements: RequirementsData, javaConfiguration, worksp
if (vmargs.indexOf(encodingKey) < 0) {
params.push(encodingKey + getJavaEncoding());
}
if (os.platform() == 'win32') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fbricon
Copy link
Collaborator

fbricon commented Apr 12, 2019

Restarting spring-boot workspace, I see the shutdown doesn't complete, and the workspace is still not properly saved:

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 15:32:09.126
!MESSAGE >> shutdown

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 15:32:09.127
!MESSAGE Saving workspace
!SESSION 2019-04-12 15:32:14.400 -----------------------------------------------
eclipse.buildId=unknown
java.version=1.8.0_181
java.vendor=Oracle Corporation
BootLoader constants: OS=macosx, ARCH=x86_64, WS=cocoa, NL=en_CA
Command-line arguments:  -data /Users/fbricon/Library/Application Support/Code - Insiders/User/workspaceStorage/4b108579b42a00cd8343838a6a610090/redhat.java/jdt_ws

!ENTRY org.eclipse.core.resources 2 10035 2019-04-12 15:32:15.906
!MESSAGE The workspace exited with unsaved changes in the previous session; refreshing workspace to recover changes.

@snjeza
Copy link
Contributor Author

snjeza commented Apr 12, 2019

Restarting spring-boot workspace, I see the shutdown doesn't complete, and the workspace is still not properly saved:

Could you test with https://raw.githubusercontent.com/snjeza/vscode-test/master/java-0.43.1.vsix ?

@fbricon
Copy link
Collaborator

fbricon commented Apr 12, 2019

This is on another workspace, but this looks much better, shutdown takes several seconds to complete, but does complete with your special build:


!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 16:25:34.321
!MESSAGE >> shutdown

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 16:25:34.321
!MESSAGE Saving workspace

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 16:25:37.599
!MESSAGE Workspace saved. Took 3277ms

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 16:25:37.600
!MESSAGE Shutdown received... waking up main thread

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 16:25:37.605
!MESSAGE class org.eclipse.jdt.ls.core.internal.JavaLanguageServerPlugin is stopping:
!SESSION 2019-04-12 16:25:39.447 -----------------------------------------------
eclipse.buildId=unknown
java.version=1.8.0_181
java.vendor=Oracle Corporation
BootLoader constants: OS=macosx, ARCH=x86_64, WS=cocoa, NL=en_CA
Command-line arguments:  -data /Users/fbricon/Library/Application Support/Code/User/workspaceStorage/4b108579b42a00cd8343838a6a610090/redhat.java/jdt_ws

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 16:25:45.133
!MESSAGE class org.eclipse.jdt.ls.core.internal.JavaLanguageServerPlugin is started

!ENTRY org.eclipse.jdt.ls.core 1 0 2019-04-12 16:25:45.261
!MESSAGE Main thread is waiting

@snjeza snjeza changed the title Fix build on Windows [WIP] Fix build on Windows Apr 12, 2019
@snjeza snjeza changed the title [WIP] Fix build on Windows Fix build on Windows Apr 12, 2019
@snjeza snjeza changed the title Fix build on Windows {WIP] Fix build on Windows Apr 21, 2019
@snjeza snjeza changed the title {WIP] Fix build on Windows Fix build on Windows Apr 23, 2019
if (os.platform() === 'win32') {
const vmargs = javaConfig.get('jdt.ls.vmargs', '');
const watchParentProcess = '-DwatchParentProcess=';
if (vmargs.indexOf(watchParentProcess) >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

watchParentProcess is always present, as per L78

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is called before the prepareParams method (L78).

Copy link
Member

@rgrunber rgrunber Sep 27, 2022

Choose a reason for hiding this comment

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

@snjeza , why do we need to set detatched to true only on Windows ? Does https://github.com/microsoft/vscode-languageserver-node/blob/e7b7e3134791c4208165aa57fb9182895df8ec6c/client/src/node/main.ts#L201 just work on Linux/MacOS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it just works. I think it is the default on Linux/Mac.

@yaohaizh
Copy link
Collaborator

@fbricon @snjeza Could this change microsoft/vscode-languageserver-node#485 fix the parent watching process issue?

@snjeza snjeza changed the title Fix build on Windows [WIP] Fix build on Windows Nov 24, 2021
@snjeza snjeza changed the title [WIP] Fix build on Windows java server refreshing the workspace for each restart on Windows Dec 4, 2021
@snjeza
Copy link
Contributor Author

snjeza commented Dec 4, 2021

I have updated the PR.

@snjeza snjeza removed the in progress label Dec 4, 2021
@jdneo
Copy link
Collaborator

jdneo commented Sep 28, 2022

@snjeza I tested this change on Windows and observed that after reloading the window, the message !MESSAGE The workspace exited with unsaved changes in the previous session; refreshing workspace to recover changes. will not be seen anymore.

One question: I think the key point is options.detached = true;, but according to current change, it will only be set when vmargs contains -DwatchParentProcess=, which is not contained in the default value of the vmargs. What's the purpose for that check?

@snjeza
Copy link
Contributor Author

snjeza commented Sep 28, 2022

One question: I think the key point is options.detached = true;, but according to current change, it will only be set when vmargs contains -DwatchParentProcess=, which is not contained in the default value of the vmargs. What's the purpose for that check?

We could set it as the default. It will not run tasklist (on Windows) or kill (on Linux/Mac)t after eclipse-jdtls/eclipse.jdt.ls#2115
This way, Java LS will be shut down on Windows properly.
@fbricon @rgrunber @testforstephen @CsCherrYY

@rgrunber
Copy link
Member

rgrunber commented Sep 28, 2022

@jdneo @snjeza How does this change solve the issue on Windows, with only defaults if -DwatchParentProcess=false is never passed as a JVM argument. It only gets set to false in prepareParams, which happens after the change in this PR. Did you manually set -DwatchParentProcess=false in java.jdt.ls.vmargs ?

@snjeza
Copy link
Contributor Author

snjeza commented Sep 28, 2022

@rgrunber you should set -DwatchParentProcess=30, 30 seconds delay

@rgrunber
Copy link
Member

If this helps the situation, then I'm fine with merging. I just don't see how it would benefit the issue without users also overriding the default java.jdt.ls.vmargs in their settings. But if a manual change to a setting is required then most users likely won't benefit.

@snjeza
Copy link
Contributor Author

snjeza commented Sep 28, 2022

If this helps the situation, then I'm fine with merging. I just don't see how it would benefit the issue without users also overriding the default java.jdt.ls.vmargs in their settings. But if a manual change to a setting is required then most users likely won't benefit.

See #880 (comment)

@jdneo
Copy link
Collaborator

jdneo commented Sep 29, 2022

When I tested this change, I manually changed the vmargs. options.detached = true; seems to work. I'm also confused the check for the -DwatchParentProcess.

@snjeza Could you help me understand the relationship between -DwatchParentProcess and options.detached = true;?

@snjeza
Copy link
Contributor Author

snjeza commented Sep 29, 2022

Could you help me understand the relationship between -DwatchParentProcess and options.detached = true;?

We have disabled watchParentProcess on Windows because Windows kills child processes when the parent process finishes.
This way, Java LS will not save a big workspace properly.
options.detached=true requires that Windows doesn't kill child processes and Java LS saves the workspace properly.
I have updated the PR. Now, it works in the following way:

Copy link
Collaborator

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

I tested on the Windows machine.

Opening eclipse.jdt.ls project:

  • Without the change, takes around 60s
  • With the change, it takes around 30s.

@testforstephen
Copy link
Collaborator

testforstephen commented Sep 29, 2022

Just found the previous background eclipse-lemminx/lemminx#328 to disable parentProcessWatcher by default. Since we've optimized the server-side parentProcessWatcher via the JDK native API, it is worth to enable it back if the performance gains are high. I suggest we put it in pre-release for a try first.

@snjeza snjeza merged commit 5643a2e into redhat-developer:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants