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

Fixing shutdown routine #53

Merged
merged 4 commits into from
Nov 23, 2017
Merged

Conversation

laughedelic
Copy link
Member

@laughedelic laughedelic commented Nov 23, 2017

Fixes #30

The problem is in the scala-json-rpc way of handling commands (requests) and notifications. It doesn't allow them to have an empty or absent params key:

https://github.com/dhpcs/scala-json-rpc/blob/c25cda4b6a39441bbe4ce25f57ab88e3db5ef635/scala-json-rpc/src/main/scala/com/dhpcs/jsonrpc/MessageCompanions.scala#L66-L71

While the LSP spec for shutdown says explicitly:

Request:

  • method: 'shutdown'
  • params: void

Same about shutdown response and exit notification. These are the only messages in LSP that don't have parameters, so I'm adding a special readoverride to handle these messages.

@laughedelic laughedelic self-assigned this Nov 23, 2017
@@ -282,7 +282,8 @@ class ScalametaLanguageServer(
} catch {
case NonFatal(e) =>
onError(e)
ShutdownResult(-1)
// FIXME: server shouldn't send shutdown response if there was no shutdown request
ShutdownResult()
Copy link
Member Author

Choose a reason for hiding this comment

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

@olafurpg I think something wrong is happening here. Server shouldn't send a shutdown response on completion request failure.

Copy link
Member

@olafurpg olafurpg Nov 23, 2017

Choose a reason for hiding this comment

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

lol, no idea how that got there. feel free to remove the whole try/catch, errors get caught and logged on request now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in f3d9342 (+ same in documentFormattingRequest)

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for fixing this, it's been annoying to see that shutdown crash on every close

@olafurpg olafurpg merged commit 893abe4 into scalameta:master Nov 23, 2017
@laughedelic laughedelic deleted the shutdown-routine branch November 23, 2017 21:52
@laughedelic
Copy link
Member Author

laughedelic commented Nov 23, 2017

Oh no... 😭 CI release is broken again. Didn't trigger on merge.
I wonder if that condition is too complicated for Travis, because the docs say:

condition: deploy when a single bash condition evaluates to true. This must be a string value, and is equivalant to if [[ <condition> ]]; then <deploy>; fi. For example, $CC = gcc.

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.

2 participants