fix: propagate LanguagServerWrapper startup exception correctly #1044
Conversation
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@ghentschke , this makes sense now to me. We have opened a discussion in #1093 just in case you think it is not.
| /** | ||
| * @return whether the last startup attempt has failed | ||
| */ | ||
| public synchronized boolean startupFailed() { |
There was a problem hiding this comment.
Is this method called by anyone? What's the purpose of this method?
There was a problem hiding this comment.
It is to determine whether a startup attempt on this LanguageServerWrapper object failed and a call to restart() is needed. We will be using it in our project.
| * Starts a language server and triggers initialization. If language server is | ||
| * started and active, does nothing. If language server is inactive, restart it. | ||
| * Starts a language server and triggers initialization. If language server has been started | ||
| * before, does nothing. | ||
| */ | ||
| public synchronized void start() { | ||
| start(false); |
There was a problem hiding this comment.
If an exception has been occurred during start. The LS cannot be started again when the cause has been fixed (e.g. wrong path do LS binary).
There was a problem hiding this comment.
I think it could we worth explaining as well that "an Eclipse-base product can check startupFailed and call restart if the problem has been programmatically handled", and that otherwise "once the problem is fixed a restart of eclipse is needed". That would clarify as well the need for the startupFailed method for future readers of this class. My wording can most probably be improved :)
There was a problem hiding this comment.
..but I am okay with it. This can/should be handled by the LS provider
| * Starts a language server and triggers initialization. If language server is | ||
| * started and active, does nothing. If language server is inactive, restart it. | ||
| * Starts a language server and triggers initialization. If language server has been started | ||
| * before, does nothing. | ||
| */ | ||
| public synchronized void start() { | ||
| start(false); |
There was a problem hiding this comment.
..but I am okay with it. This can/should be handled by the LS provider
If there is an exception in the startup of the LanguageServerWrapper, consumers get a cancellation exception rather than the actual exception that occurred. With this commit, this behavior is changed. As a consequence of the change, calls to LanguageServerWrapper.start() will do nothing after a failure. To test for this case (and attempt a restart() if feasible), the startupFailed() method is added.
|
Thanks for the improved javadoc |
|
It looks like there is a need for investigation of view the CI Jenkins job fails more or less consistently. Other PRs seem to be fine in the same job, so it does not look like a infrastructure glitch. |
If there is an exception in the startup of the LanguageServerWrapper, consumers get a cancellation exception rather than the actual exception that occurred. This commit fixes the problem.