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

Make LSP server shutdown configurable #885

Closed
kaikreuzer opened this issue Oct 1, 2018 · 7 comments

Comments

@kaikreuzer
Copy link
Member

commented Oct 1, 2018

We (that is the Eclipse SmartHome project) recently upgraded to Xtext 2.14 and now ran into the issue that our server is silently shutting down, if VS Code is disconnecting after having used LSP.
We tracked that down to #474 and being a "feature" of an LSP server. While I see that this is a part of the specification and thus has to be supported, it is nonetheless an absolutely no-go to call System.exit() from within some library, especially within an OSGi container.
I guess our situation, where LSP is run as a service on an existing server (and not as a JVM process on its own), clearly shows that this is a pretty nasty behavior.

Imho, the clean way to implement this in an OSGi way would be to provide a hook for listening to exit requests and have a default implementation of such a listener, which cleanly shuts down the OSGi container, while giving consumers the chance to deactivate/replace this default implementation (e.g. through config admin). Wdyt?

@kthoms

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

I understand your concern, but as you already found out it is a feature. Your requirement is to shut down the OSGi container then, and a listener could be a way to do so. However, the Xtext core bundles are not aware of OSGi and don't have a dependency on that. And I do not think that one should be introduced. This means it must be somehow added from outside.

I could imagine to delegate the behaviour to a shutdown handle that could be overridden by a specific language. Or do you have an idea how to shut down the OSGi container without a dependency here?

@kaikreuzer

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

Your requirement is to shut down the OSGi container then

No, not at all - at most, the LSP server bundles should stop their services and must not impact any other code running on the OSGi container. Btw, just for my understanding, what is the background of this "feature"? If the server is stopped upon closing the editor, how is it started when opening the editor again?

This means it must be somehow added from outside.

Well, the concept of registering listeners (addExitNotificationListener method) isn't related to OSGi, so this "logic" can easily be drawn out of the code code. And the default listener implementation could e.g. be deactivated by a system property (to not have any OSGi dependency either).

Btw, in any case, the code should properly handle Security exceptions. When using a Java security manager, you currently get something like this in your log:

Oct 01, 2018 9:25:16 AM org.eclipse.lsp4j.jsonrpc.RemoteEndpoint handleNotification
WARNING: Notification threw an exception: {
  "jsonrpc": "2.0",
  "method": "exit",
  "params": null
}
java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$null$0(GenericEndpoint.java:63)
	at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.notify(GenericEndpoint.java:148)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleNotification(RemoteEndpoint.java:216)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.consume(RemoteEndpoint.java:183)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.handleMessage(StreamMessageProducer.java:188)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:90)
	at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:95)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.eclipse.lsp4j.jsonrpc.services.GenericEndpoint.lambda$null$0(GenericEndpoint.java:61)
	... 11 more
Caused by: java.lang.SecurityException: Prevented System.exit() call.
	at org.openhab.core.internal.CoreActivator$1.checkPermission(CoreActivator.java:51)
	at java.lang.SecurityManager.checkExit(SecurityManager.java:761)
	at java.lang.Runtime.exit(Runtime.java:107)
	at java.lang.System.exit(System.java:971)
	at org.eclipse.xtext.ide.server.LanguageServerImpl.exit(LanguageServerImpl.java:334)
	... 16 more

(just to repeat my statement from above: " it is an absolute no-go to call System.exit() from within some library".)

@cdietrich

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

@svenefftinge please comment what you think regarding this issue. Adding a new listener is bad since it breaks api. What about a new callback that defaults to calling system.exit

@kaikreuzer

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

the code should properly handle Security exceptions

FTR, the server might be in some limbo state if the security manager prevented the system exit as the LSP server might remain in "being shut down" state. (I couldn't clearly reproduce this, see this comment.)

@svenefftinge

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

I agree with Kai in that we should not call System.exit down in a library. Instead we should provide a onExit callback that is provided from the starter code.

@kthoms

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

Could you work on that?

@cdietrich cdietrich added this to the Release_2.16 milestone Oct 9, 2018

@cdietrich cdietrich self-assigned this Oct 9, 2018

cdietrich added a commit that referenced this issue Oct 9, 2018
[#885] make LSP server exit behaviour configurable
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
cdietrich added a commit that referenced this issue Oct 18, 2018
[#885] make LSP server exit behaviour configurable
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
cdietrich added a commit that referenced this issue Oct 18, 2018
[#885] make LSP server exit behaviour configurable
Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
cdietrich added a commit that referenced this issue Oct 18, 2018
Merge pull request #896 from eclipse/cd_issue885
[#885] make LSP server exit behaviour configurable
@cdietrich

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Fixed in 2.16

@cdietrich cdietrich closed this Oct 18, 2018

wborn added a commit to wborn/openhab-core that referenced this issue Apr 17, 2019
Configure LanguageServer exit behavior
Xtext 2.17 allows for configuring the LanguageServer exit behavior so the SecurityManager workaround can be removed.

Fixes: eclipse/smarthome#6291
Undos: openhab#409

See also:
* "Configurable Server Exit Behavior" in the Xtext 2.16.0 release notes:
  https://www.eclipse.org/Xtext/releasenotes.html#/releasenotes/2018/12/04/version-2-16-0
* eclipse/xtext-core#885

Signed-off-by: Wouter Born <github@maindrain.net>
maggu2810 added a commit to openhab/openhab-core that referenced this issue Apr 17, 2019
Configure LanguageServer exit behavior (#742)
Xtext 2.17 allows for configuring the LanguageServer exit behavior so the SecurityManager workaround can be removed.

Fixes: eclipse/smarthome#6291
Undos: #409

See also:
* "Configurable Server Exit Behavior" in the Xtext 2.16.0 release notes:
  https://www.eclipse.org/Xtext/releasenotes.html#/releasenotes/2018/12/04/version-2-16-0
* eclipse/xtext-core#885

Signed-off-by: Wouter Born <github@maindrain.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.