Skip to content

Close stray processes on exit #663

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

Merged

Conversation

TylerLeonhardt
Copy link
Member

Fixes #655

This shuts down PSES when it receives the exit message following the language server protocol.

This also makes the shutdown message NOT kill the server which also follows the language server protocol.

I've tested this using the NeoVim Language Client

Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can you explain how it works with such a small change?

@@ -70,7 +70,7 @@ public class EditorServicesHost
private IServerListener languageServiceListener;
private IServerListener debugServiceListener;

private TaskCompletionSource<bool> serverCompletedTask;
private TaskCompletionSource<bool> serverCompletedTask = new TaskCompletionSource<bool>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is also assigned in the constructor. Is there a need to initialise it here as well? Can we initialise it in the constructor instead of here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's assigned in the constructor of LanguageServer not EditorServicesHost.

I could move the assignment to the constructor of EditorServices as well though - like featureFlags.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -146,6 +155,7 @@ protected Task Stop()
Logger.Write(LogLevel.Normal, "Language service is shutting down...");

// TODO: Raise an event so that the host knows to shut down
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the TODO is done?

Copy link
Member Author

Choose a reason for hiding this comment

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

rip

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@TylerLeonhardt
Copy link
Member Author

So the way the language server stays running is because of this line:
https://github.com/PowerShell/PowerShellEditorServices/blob/master/module/PowerShellEditorServices/Start-EditorServices.ps1#L330

$editorServicesHost.WaitForCompletion()

That function really just waits for serverCompletedTask to be "complete"
https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Host/EditorServicesHost.cs#L352

So what I've done is plumbed that CompletionTask down to where we receive the exit lsp message and trigger that CompletionTask to complete.

When the task completes, WaitForCompletion() finishes executing and thus, PowerShell itself finishes executing Start-EditorServices.ps1 and exits.

@@ -156,7 +166,6 @@ protected Task Stop()
RequestContext<object> requestContext)
{
// Allow the implementor to shut down gracefully
await this.Stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like we are simply ignoring the ShutdownRequest message? Does this mean we are really only processing the ExitNotification message in order to stop the lang server?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct. The shutdown message is not supposed to actually do things that would stop the process.

It's the message that gets sent to inform the language server that a shutdown is occurring and that you should expect an exit message soon.

Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants