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 LS should lock its workspace #1010

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Apr 25, 2019

Fixes #978

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

@snjeza snjeza requested review from gorkem and fbricon April 25, 2019 16:47
Copy link
Contributor

@fbricon fbricon left a comment

Choose a reason for hiding this comment

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

I could only test it in vscode by inverting the lock check condition. So when it fails, vscode shows no log message (you need to manually open the workspace .log file) and the only user feedback is the server failed to start 5 times.
@snjeza if the server actually crashes, what are the chances the lock survives after a restart?

Can someone from the Che team test this PR? cc @tsmaeder , @tolusha

if (workspaceFile.exists()) {
JavaLanguageServerPlugin.logError("Could not launch the server because associated workspace is currently in use by another Java LS server.");
}
return IApplication.EXIT_OK;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should return a Not-OK code instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

if (!instanceLoc.lock()) {
File workspaceFile = new File(instanceLoc.getURL().getFile());
if (workspaceFile.exists()) {
JavaLanguageServerPlugin.logError("Could not launch the server because associated workspace is currently in use by another Java LS server.");
Copy link
Contributor

Choose a reason for hiding this comment

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

at that point there's no connection established so no message can surface to the client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can only use the standard Eclipse log (/.metadata/.log)

File workspaceFile = new File(instanceLoc.getURL().getFile());
if (workspaceFile.exists()) {
JavaLanguageServerPlugin.logError("Could not launch the server because associated workspace is currently in use by another Java LS server.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if workspaceFile doesn't exist we don't log anything???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can try add the following property:

java.jdt.ls.vmargs": "-Djdt.ls.debug=true...",

Copy link
Contributor

Choose a reason for hiding this comment

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

my point is you have a workflow branch where the server would exit without logging anything whatsoever. Makes it harder to troubleshoot

@snjeza
Copy link
Contributor Author

snjeza commented Apr 30, 2019

test this please

if (workspaceFile.exists()) {
JavaLanguageServerPlugin.logError("Could not launch the server because associated workspace is currently in use by another Java LS server.");
}
return new Integer(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Integer.valueOf(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@fbricon
Copy link
Contributor

fbricon commented May 17, 2019

So what happens with multiple tabs on the same in Che workspace, when that PR is applied?

@snjeza
Copy link
Contributor Author

snjeza commented May 17, 2019

Only one Java LS will be started. Che 7 will receive an error message.

@fbricon
Copy link
Contributor

fbricon commented May 17, 2019

Che 7 will receive an error message.

Can you provide a screenshot? I'm curious to see that.
@tsmaeder is it possible for several people to work on the same workspace simultaneously?

@snjeza
Copy link
Contributor Author

snjeza commented May 17, 2019

Can you provide a screenshot? I'm curious to see that.

I will try.

@tsmaeder
Copy link
Contributor

@tsmaeder is it possible for several people to work on the same workspace simultaneously?

As with Theia itself, you can open multiple windows into the same back end. But that's not the worst: if you restart the language server (by changing the vm options in the preferences, for example), the old processes seem to coexist with the new ones, so I suspect this is a real problem.

While refusing to work on a locked workspace is probably the correct thing to do when looking only at jdt.ls, this will probably break stuff.
Again, What does VS Code do to prevent this? Do you have separate state locations for multiple copies of VS Code running?

@snjeza
Copy link
Contributor Author

snjeza commented May 20, 2019

Again, What does VS Code do to prevent this? Do you have separate state locations for multiple copies of VS Code running?

You can try the "Duplicate Workspace in New Window" command. It will start two Java LS servers with different Java LS workspaces.
See https://github.com/microsoft/vscode/blob/master/src/vs/workbench/browser/actions/workspaceActions.ts#L282

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.

Java LS should lock its workspace
3 participants