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.

Sign up for GitHub

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

Jump to bottom

LSP server broken when using stream method #1708

Closed
rchl opened this issue Nov 3, 2020 · 2 comments · Fixed by #1710
Closed

LSP server broken when using stream method #1708

rchl opened this issue Nov 3, 2020 · 2 comments · Fixed by #1710

Comments

@rchl
Copy link
Contributor

rchl commented Nov 3, 2020 •
edited
Loading

Run server with:

./node_modules/graphql-language-service-cli/bin/graphql.js server --method stream

and send the initialize message to it.

The server will just crash in a non-crashy way (just exit with 0 exit code).

This issue was introduced by https://github.com/graphql/graphiql/pull/1651/files where it has changed the logger to post to stdout for non-Error messages:

const processSt = severity === DIAGNOSTIC_SEVERITY.Error ? process.stderr : process.stdout;

This doesn't work for stream method as stdio is reserved for communicating with the LSP client.

@acao
@rchl rchl mentioned this issue Nov 3, 2020
Closed
@acao
Copy link
Member

acao commented Nov 4, 2020

thanks for looking into this! excellent deduction as well. would you like to open a PR that ensures this works for streams? you should be able to pass the options down to Logger readily since it's instantiated in createServer, and then change the logic you referenced. When developing yarn watch to set the build watcher for language server/etc, and then you can just directly execute the cli bin path to test after every change, since yarn workspace takes care of all the local interdepdent resolutions. I won't have time to get to this for a while

since our tests and my manual testing is always with ipc, i've probably let the stream interface languish, my bad!

@rchl
Copy link
Contributor Author

rchl commented Nov 4, 2020

Thanks for providing helpful information. I might take a stab at it.

rchl added a commit to rchl/graphiql that referenced this issue Nov 4, 2020
@rchl
fix: crash on receiving an LSP message in "stream" mode
8f6200d 
Don't write log messages to stdout in "stream" mode. stdout is reserved
for communicating with the LSP client in the "stream" mode so in that
mode only write to stderr.

Fixes lsp server crashing on first message it receives (or maybe LSP
client closing it because it received invalid response).

Also:
 - the callback to socket.write() was always logging the "err" parameter
   even if there was undefined which caused spurious lines in the
   console. Only actually log that if "err" is truthy.
 - Avoid too many newlines at the end of the log message as that also
   causes spurious empty lines in the log and the console.
 - Gracefully handle case when the response for "workspace/configuration"
   returns null value. Those configuration options have sane defaults
   and don't need to be present.
 - The Logger._stream property was removed as it was unused.

Resolves graphql#1708
rchl added a commit to rchl/graphiql that referenced this issue Nov 4, 2020
@rchl
fix: crash on receiving an LSP message in "stream" mode
fe80238 
Don't write log messages to stdout in "stream" mode. stdout is reserved
for communicating with the LSP client in the "stream" mode. Write all
types of log messages to stderr.

Fixes LSP server crashing on the first message it receives (or maybe
LSP client closing it because it received an invalid response).

Also:
 - the callback to socket.write() was always logging the "err" parameter
   even if there was undefined which caused spurious lines in the
   console. Only actually log that if "err" is truthy.
 - Avoid too many newlines at the end of the log message as that also
   causes spurious empty lines in the log and the console.
 - Gracefully handle the case when the response for "workspace/configuration"
   returns a null value. Those configuration options have sane defaults
   and don't need to be present.
 - The Logger._stream property was removed as it was unused.

Resolves graphql#1708
This was referenced Nov 4, 2020
Merged
Closed
rchl added a commit to rchl/graphiql that referenced this issue Nov 14, 2020
@rchl
fix: crash on receiving an LSP message in "stream" mode
f28ff4e 
Don't write log messages to stdout in "stream" mode. stdout is reserved
for communicating with the LSP client in the "stream" mode. Write all
types of log messages to stderr.

Fixes LSP server crashing on the first message it receives (or maybe
LSP client closing it because it received an invalid response).

Also:
 - the callback to socket.write() was always logging the "err" parameter
   even if there was undefined which caused spurious lines in the
   console. Only actually log that if "err" is truthy.
 - Avoid too many newlines at the end of the log message as that also
   causes spurious empty lines in the log and the console.
 - Gracefully handle the case when the response for "workspace/configuration"
   returns a null value. Those configuration options have sane defaults
   and don't need to be present.
 - The Logger._stream property was removed as it was unused.

Resolves graphql#1708
@snyk-bot snyk-bot mentioned this issue Nov 17, 2020
Open
acao pushed a commit that referenced this issue Nov 23, 2020
@rchl @acao
fix: crash on receiving an LSP message in "stream" mode
1238075 
Don't write log messages to stdout in "stream" mode. stdout is reserved
for communicating with the LSP client in the "stream" mode. Write all
types of log messages to stderr.

Fixes LSP server crashing on the first message it receives (or maybe
LSP client closing it because it received an invalid response).

Also:
 - the callback to socket.write() was always logging the "err" parameter
   even if there was undefined which caused spurious lines in the
   console. Only actually log that if "err" is truthy.
 - Avoid too many newlines at the end of the log message as that also
   causes spurious empty lines in the log and the console.
 - Gracefully handle the case when the response for "workspace/configuration"
   returns a null value. Those configuration options have sane defaults
   and don't need to be present.
 - The Logger._stream property was removed as it was unused.

Resolves #1708
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: crash on receiving an LSP message in "stream" mode rchl/graphiql
2 participants
@rchl @acao