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 #53

Closed
manuEbg opened this issue Jun 21, 2021 · 10 comments
Closed

Shutdown #53

manuEbg opened this issue Jun 21, 2021 · 10 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@manuEbg
Copy link
Contributor

manuEbg commented Jun 21, 2021

As user, I can use the LSP console application to shut down the server gracefully, so that all processes on the client and server side are stopped without any error messages or warnings. (Ignoring warnings does not count 😜)

I just leave this here...

@manuEbg manuEbg added the enhancement New feature or request label Jun 21, 2021
@manuEbg manuEbg added this to the Prototype 3 milestone Jun 21, 2021
@manuEbg manuEbg assigned manuEbg and IlmarB and unassigned manuEbg Jun 21, 2021
@CSchoel
Copy link
Contributor

CSchoel commented Jun 25, 2021

As a reference, you can have a look at the implementation in my test project.

The additional challenge is to do this in two steps: shutdown() should only stop the server from sending anything other than information messages, and only when the server receives the exit() call it should actually exit.

@CSchoel
Copy link
Contributor

CSchoel commented Jun 28, 2021

@IlmarB This issue has not received any updates in the last week. What is your status?

@IlmarB
Copy link
Contributor

IlmarB commented Jun 28, 2021

I have not fixed this yet. In fact I also thought about splitting this issue into sub-issues where one of them is reviewing your test project.

@IlmarB
Copy link
Contributor

IlmarB commented Jul 4, 2021

right now the server removes the client gracefully but does not stop which should be fixed next.

@CSchoel
Copy link
Contributor

CSchoel commented Jul 5, 2021

Two thoughts on this:

  1. Right now you disconnect the client right when a shutdown() call has been issued. However, this should only happen after exit() has been called. Otherwise this might lead to errors on the client side, because the client either does not receive an answer to the shutdown request or is unable to send the exit request.
  2. Since you have a setup that potentially uses multiple clients, is it really a good idea to shut down the server if one of the potentially many clients exits?

@manuEbg
Copy link
Contributor Author

manuEbg commented Jul 5, 2021

Let me summarize the plan we agreed on at the beginning of the sprint again:

  1. We need two Commands in the ConsoleClient Exit and Shutdown Server
  2. The Exit Command just disconnects the Client from the Server in a gracefull way and terminates the ClientProcess
  3. The Shutdown Command triggers the LspShutdownCommand of the server which could do things like:
  • Notify other connected Clients about shutdown,
  • Terminates OMC-Process
  • Maybe some other Stuff in the Future
  1. After the Client who sends the ShutdownRequest receives his answer from the Server the (Lsp)Exit Notification gets sended
  2. The Exit-Notification should do Things like gracefully disconnecting all the connected Clients and terminating the ServerProcess

@CSchoel
Copy link
Contributor

CSchoel commented Jul 5, 2021

Yes, that sums it up nicely. I just would rename the Exit command to something like Disconnect, so that the name is more telling of what the command actually does and there is no name confusion with the LSP commands shutdown and exit.

What I was getting at with my second point was the fact that I am not sure whether you can prevent a standard LSP client like the vscode implementation from sending LSP shutdown commands. All the shutdown and cleanup is done in a function called client.stop() which both sends shutdown and exit via LSP. In order to use the setup that you posted, we would have to disentangle this and manually cleanup the resources in the vscode client without calling client.stop() when we use the Disconnect action.

Maybe you should clarify what use cases you want to cover with the multi-client feature. Should something like the following be possible?

  1. Server starts
  2. Vscode client 1 starts, connects to server
  3. Vscode client 2 starts, connects to server
  4. ... (Vscode clients 1 and 2 send some Mo|E and LSP commands)
  5. Vscode client 2 stops (sends shutdown and then exit)
  6. Vscode client 1 continues sending Mo|E and LSP commands

If not, what about this scenario?

  1. Server starts
  2. Vscode client 1 starts, connects to server
  3. ... (Vscode client 1 sends some Mo|E and LSP commands)
  4. Vscode client 1 stops (sends shutdown and then exit)
  5. Vscode client 1 restarts, re-connects to server
  6. Vscode client 1 continues sending Mo|E and LSP commands

Both in the first and in the second scenario you have the issue that stopping a vscode client is synonymous with sending shutdown and exit, but in the current setup that you posted, sending shutdown disables the server from working with any of the other potentially still connected clients and/or future clients that want to connect to the server instance. If you want to neither support the first nor the second scenario then what is the point of the multi-client feature? Should only the console client be allowed as an "extra" client next to a vscode client?

@manuEbg
Copy link
Contributor Author

manuEbg commented Jul 7, 2021

As I understand it, stopping a client does not imply sending the shutdown and exit command

@IlmarB
Copy link
Contributor

IlmarB commented Jul 11, 2021

Done - 2 Options are now available
9: Exit - Shutdown server (for a full shutdown)
10: Exit - Disconnect (Client stops, server continues running)

@IlmarB IlmarB closed this as completed Jul 11, 2021
@CSchoel
Copy link
Contributor

CSchoel commented Jul 12, 2021

As I understand it, stopping a client does not imply sending the shutdown and exit command

Not in LSP4J, but in vscode-languageserver-node you have the stop() method as the only available top-level way of disconnecting from a server and cleaning up resources. As you can see, it calls shutdown and exit on the connection object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants