Skip to content

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

Merged
snjeza merged 1 commit into
redhat-developer:masterfrom
snjeza:issue-793
Sep 29, 2022
Merged

java server refreshing the workspace for each restart on Windows #880
snjeza merged 1 commit into
redhat-developer:masterfrom
snjeza:issue-793

Conversation

@snjeza

@snjeza snjeza commented Apr 12, 2019

Copy link
Copy Markdown
Collaborator

Fixes #793

Signed-off-by: Snjezana Peco snjezana.peco@redhat.com

Comment thread src/javaServerStarter.ts Outdated

Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@fbricon

fbricon commented Apr 12, 2019

Copy link
Copy Markdown
Collaborator

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

snjeza commented Apr 12, 2019

Copy link
Copy Markdown
Collaborator Author

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

fbricon commented Apr 12, 2019

Copy link
Copy Markdown
Collaborator

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
Comment thread src/javaServerStarter.ts Outdated

Copy link
Copy Markdown
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
Copy Markdown
Collaborator 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).

@rgrunber rgrunber Sep 27, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
Collaborator 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
Copy Markdown
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

snjeza commented Dec 4, 2021

Copy link
Copy Markdown
Collaborator Author

I have updated the PR.

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

jdneo commented Sep 28, 2022

Copy link
Copy Markdown
Collaborator

@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

snjeza commented Sep 28, 2022

Copy link
Copy Markdown
Collaborator Author

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

rgrunber commented Sep 28, 2022

Copy link
Copy Markdown
Member

@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

snjeza commented Sep 28, 2022

Copy link
Copy Markdown
Collaborator Author

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

@rgrunber

Copy link
Copy Markdown
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

snjeza commented Sep 28, 2022

Copy link
Copy Markdown
Collaborator Author

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

jdneo commented Sep 29, 2022

Copy link
Copy Markdown
Collaborator

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

snjeza commented Sep 29, 2022

Copy link
Copy Markdown
Collaborator Author

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:

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

@jdneo jdneo left a comment

Copy link
Copy Markdown
Collaborator

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

testforstephen commented Sep 29, 2022

Copy link
Copy Markdown
Collaborator

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

Development

Successfully merging this pull request may close these issues.

Windows 10 x64/macOS, open spring-boot/wildfly, it will always trigger full build after initialized after imported

6 participants