Skip to content

PSES needs to shutdown itself when it's supposed to #655

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

Closed
TylerLeonhardt opened this issue Apr 21, 2018 · 7 comments
Closed

PSES needs to shutdown itself when it's supposed to #655

TylerLeonhardt opened this issue Apr 21, 2018 · 7 comments
Labels

Comments

@TylerLeonhardt
Copy link
Member

Ok everyone, I did some research and here's what I've found:

Right now, the client kills the server process.

The proof can be found here in the VSCode client: https://github.com/PowerShell/vscode-powershell/blob/master/src/process.ts#L145-L161

That said, this is not a good thing.

You might be wondering... The LSP has both a shutdown and an exitmessage, don't we implement those?

The answer is yes.......... but it's functionally a no-op 😢

You can see the TODOcomment here:
https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs#L148

which is referring to this TaskCompletionSource that is causing the process to stay open:

this.serverCompletedTask = new TaskCompletionSource<bool>();

All we need to do is pass that TaskCompletionSource into the handler and complete the source when an exit event is received (possibly when there are 0 connections)

P.s. big thanks to @SeeminglyScience for helping me investigate this! 😄

@rjmholt rjmholt changed the title PSES needs to shutdown itself when it's suppose to PSES needs to shutdown itself when it's supposed to Apr 22, 2018
@nanoant
Copy link

nanoant commented Apr 23, 2018

Also I would recommend to PSES to shutdown itself when all communication channels are closed, e.g. when stdin pipe is closed and there is no way to talk with PSES anymore.
Still sending exit would be nice graceful way for clients to issue the termination request.

@rjmholt
Copy link
Contributor

rjmholt commented Apr 23, 2018

Given that a single PSES instance will only ever talk to one client, but that that client can communicate with PSES over multiple channels (which I assume are all connection-oriented, guaranteed-delivery channels, so that we get sent an interrupt or a message when they are closed/broken), would the following behaviour make sense:

  • PSES keeps a list of all open channels, when they all close it shuts down.
  • If the client sends an exit message on any (or the relevant) channel, PSES closes all channels and shuts down.
  • If a single channel is closed but others remain open PSES keeps going.
  • If a single channel fails but others remain open PSES keeps going, and it's up to the client to reopen the channel.

?

@yatli
Copy link

yatli commented Apr 24, 2018

@rjmholt would you please elaborate on the open part? consider the whole lifecycle as follows:

PSES starts --> PSES setting up channel listeners ---> PSES waiting for incoming connection ---> The client ignores this channel, keep listening
                                    `
                                     `---------> PSES directly accepts stdio, but that's only a single channel ----> Client showing up here, do this and that, and then exits. Channel broken. ---> [ ? ]

Do we still consider it open?

@rjmholt
Copy link
Contributor

rjmholt commented Apr 24, 2018

@yatli I don't quite follow your example.

When you say the client ignores a channel, do you mean it doesn't try to connect to it at all? Or do you mean the service tries to connect to the client and the client ignores the request?

I've done some reading on TCP channels, and realise I had a misconception of the guarantees provided by TCP (and I imagine pipes/stdio as well). (I thought the underlying protocol guaranteed knowledge of a broken connection, like it guarantees delivery).

In which case we must send data over the channel with an expected response timeout to detect if it has broken. So a heartbeat.

The problem there is that clients that don't implement a heartbeat response will just see PSES shutdown after a single timeout duration (I imagine ~5s).

So... Perhaps we need yet another startup flag that defaults to "yes use a heartbeat" but which can also be set to "no, no heartbeat. I promise to shut down PSES myself".

@yatli
Copy link

yatli commented Apr 24, 2018

@rjmholt I mean the client ignores the channel and does not connect at all.

@TylerLeonhardt
Copy link
Member Author

Ok so good news...

When the client disconnects, PSES throws an exception that the stdio stream has closed:

https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Protocol/MessageProtocol/ProtocolEndpoint.cs#L343

So I thought this would be a great place to complete the following tasks that seem to be holding up the process from exiting:
https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Protocol/MessageProtocol/ProtocolEndpoint.cs#L100
And
https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Host/EditorServicesHost.cs#L352

... But it's still not shutting down... There has to be something else lingering...

@TylerLeonhardt
Copy link
Member Author

jk PR out 😄

#663

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

No branches or pull requests

4 participants