MopeSWTP-SS21 / MopeSWTP

MIT License
1 stars 0 forks source link

Shutdown #53

Closed manuEbg closed 3 years ago

manuEbg commented 3 years ago

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...

CSchoel commented 3 years ago

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 commented 3 years ago

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

IlmarB commented 3 years ago

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 commented 3 years ago

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

CSchoel commented 3 years ago

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 commented 3 years ago

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:
  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 commented 3 years ago

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 commented 3 years ago

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

IlmarB commented 3 years ago

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

CSchoel commented 3 years ago

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.