-
Notifications
You must be signed in to change notification settings - Fork 237
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
Changes from 3 commits
d42a142
f2a1e2e
2ce3349
e2c704a
56d1a84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,22 +40,31 @@ public class LanguageServer | |
private Dictionary<string, Dictionary<string, MarkerCorrection>> codeActionsPerFile = | ||
new Dictionary<string, Dictionary<string, MarkerCorrection>>(); | ||
|
||
private TaskCompletionSource<bool> serverCompletedTask; | ||
|
||
public IEditorOperations EditorOperations | ||
{ | ||
get { return this.editorOperations; } | ||
} | ||
|
||
/// <param name="hostDetails"> | ||
/// Provides details about the host application. | ||
/// </param> | ||
/// <summary> | ||
/// Initializes a new language server that is used for handing language server protocol messages | ||
/// </summary> | ||
/// <param name="editorSession">The editor session that handles the PowerShell runspace</param> | ||
/// <param name="messageHandlers">An object that manages all of the message handlers</param> | ||
/// <param name="messageSender">The message sender</param> | ||
/// <param name="serverCompletedTask">A TaskCompletionSource<bool> that will be completed to stop the running process</param> | ||
/// <param name="logger">The logger</param> | ||
public LanguageServer( | ||
EditorSession editorSession, | ||
IMessageHandlers messageHandlers, | ||
IMessageSender messageSender, | ||
TaskCompletionSource<bool> serverCompletedTask, | ||
ILogger logger) | ||
{ | ||
this.Logger = logger; | ||
this.editorSession = editorSession; | ||
this.serverCompletedTask = serverCompletedTask; | ||
// Attach to the underlying PowerShell context to listen for changes in the runspace or execution status | ||
this.editorSession.PowerShellContext.RunspaceChanged += PowerShellContext_RunspaceChanged; | ||
this.editorSession.PowerShellContext.ExecutionStatusChanged += PowerShellContext_ExecutionStatusChanged; | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rip There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
this.serverCompletedTask.SetResult(true); | ||
|
||
return Task.FromResult(true); | ||
} | ||
|
@@ -156,7 +166,6 @@ private async Task HandleShutdownRequest( | |
RequestContext<object> requestContext) | ||
{ | ||
// Allow the implementor to shut down gracefully | ||
await this.Stop(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. The It's the message that gets sent to inform the language server that a shutdown is occurring and that you should expect an |
||
|
||
await requestContext.SendResult(new object()); | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed