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

Shutdown the server on shutdown request using connection.handle_shutdown. #195

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

Rutherther
Copy link
Contributor

I noticed in an example in rust-analyzer repository: https://github.com/rust-lang/rust-analyzer/blob/master/lib/lsp-server/examples/goto_def.rs#L86C31-L86C46 that there is a handle_shutdown method that does a little more than what vhdl_ls currently did. Using this method also fixes #194 thanks to not actually deserializing the request - so it kind of works around the problem, so probably not the best solution.

It looks like the parameters of the shutdown request that are sent are: {}, this gets parsed into Map instead of unit - ().

@Rutherther Rutherther changed the title Shutdown the server on shutdown request using `connection.handle_shutdown. Shutdown the server on shutdown request using connection.handle_shutdown. Aug 26, 2023
@kraigher
Copy link
Member

kraigher commented Sep 8, 2023

I wrote a comment on the MR, but overall looks ok. I see the CI fails, it is because github upgraded the rust version which introduces new cargo fmt and clippy errors. I fixed those on master so you could rebase on master to get green branch.

@Rutherther
Copy link
Contributor Author

Hello, sorry for the delay, I somehow missed the comment. Unfortunately, I do not see your comment on the MR. I rebased onto master.

Err(err) => panic!("{err:?}")
}

self.handle_request(&mut server, request)
Copy link
Member

Choose a reason for hiding this comment

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

Should this code be in the else-branch of the if shutdown above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that seems better than the current solution. I have removed the break; and added else-branch with handle_request. Is this okay from your side? I have tested this in emacs and works okay, doesn't err on shutdown as it did before.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good

@kraigher
Copy link
Member

kraigher commented Sep 9, 2023

I forgot to publish the comment. Now it is published.

@Rutherther
Copy link
Contributor Author

Should I write a unit test that would send {} instead of empty body to make sure this is covered in the future? I am also not sure what the correct behavior should be when {} is received. Is this a bug on the client, or can an empty body be signaled using {}, and it's bug of the command parser then?

@Rutherther
Copy link
Contributor Author

Sorry, forgot to run fmt 🤦‍♂️, I always forget.

@kraigher
Copy link
Member

kraigher commented Sep 9, 2023

I do not think a unit test is necessary in this codebase. Since the deserialization of JSON RPC messages is handled by a third party library. It would be up to this library to support every legal shutdown request format (or be tolerant to slightly malformed messages).

@kraigher kraigher merged commit 3d64905 into VHDL-LS:master Sep 9, 2023
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.

Emacs getting stderr when restarting workspace
2 participants