From e2688a8149005669bc191cf9ecb1824b201ae3c4 Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Tue, 1 Mar 2016 20:24:50 -0700 Subject: [PATCH 1/3] This is a *proposed* change to address the issue that the launch request could come after the configurationDone request (https://github.com/Microsoft/vscode/issues/3548). This commit "should" allow the messages to come in either order and still work. However, it needs a good review. BTW it appears all the messages are processed on the same thread (or maybe that was coincidence). If that is the case then perhaps the lock is not needed. Still it is a fairly lightweight lock - one bool check and an assignment. --- .../Server/DebugAdapter.cs | 98 +++++++++++++------ 1 file changed, 68 insertions(+), 30 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs index ac445ef83..44273f94d 100644 --- a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs +++ b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs @@ -20,6 +20,9 @@ public class DebugAdapter : DebugAdapterBase { private EditorSession editorSession; private OutputDebouncer outputDebouncer; + private object syncLock = new object(); + private bool isConfigurationDoneRequestComplete; + private bool isLaunchRequestComplete; private string scriptPathToLaunch; private string arguments; @@ -65,6 +68,23 @@ protected override void Initialize() this.SetRequestHandler(EvaluateRequest.Type, this.HandleEvaluateRequest); } + protected Task LaunchScript(RequestContext requestContext) + { + return editorSession.PowerShellContext + .ExecuteScriptAtPath(this.scriptPathToLaunch, this.arguments) + .ContinueWith( + async (t) => { + Logger.Write(LogLevel.Verbose, "Execution completed, terminating..."); + + await requestContext.SendEvent( + TerminatedEvent.Type, + null); + + // Stop the server + this.Stop(); + }); + } + protected override void Shutdown() { // Make sure remaining output is flushed before exiting @@ -81,6 +101,31 @@ protected override void Shutdown() #region Built-in Message Handlers + protected async Task HandleConfigurationDoneRequest( + object args, + RequestContext requestContext) + { + // Ensure that only the second message between launch and + // configurationDone - actually launches the script. + lock (syncLock) + { + if (!this.isLaunchRequestComplete) + { + this.isConfigurationDoneRequestComplete = true; + } + } + + // The order of debug protocol messages apparently isn't as guaranteed as we might like. + // Need to be able to handle the case where we get configurationDone after launch request + // and vice-versa. + if (this.isLaunchRequestComplete) + { + this.LaunchScript(requestContext); + } + + await requestContext.SendResult(null); + } + protected async Task HandleLaunchRequest( LaunchRequestArguments launchParams, RequestContext requestContext) @@ -114,14 +159,32 @@ protected async Task HandleLaunchRequest( Logger.Write(LogLevel.Verbose, "Script arguments are: " + arguments); } - // NOTE: We don't actually launch the script in response to this - // request. We wait until we receive the configurationDone request - // to actually launch the script under the debugger. This gives - // us and VSCode a chance to finish configuring all the types of - // breakpoints. + // We may not actually launch the script in response to this + // request unless it comes after the configurationDone request. + // If the launch request comes first, then stash the launch + // params so that the subsequent configurationDone request can + // launch the script. this.scriptPathToLaunch = launchParams.Program; this.arguments = arguments; + // Ensure that only the second message between launch and + // configurationDone - actually launches the script. + lock (syncLock) + { + if (!this.isConfigurationDoneRequestComplete) + { + this.isLaunchRequestComplete = true; + } + } + + // The order of debug protocol messages apparently isn't as guaranteed as we might like. + // Need to be able to handle the case where we get configurationDone after launch request + // and vice-versa. + if (this.isConfigurationDoneRequestComplete) + { + this.LaunchScript(requestContext); + } + await requestContext.SendResult(null); } @@ -133,31 +196,6 @@ protected Task HandleAttachRequest( throw new NotImplementedException(); } - protected async Task HandleConfigurationDoneRequest( - object args, - RequestContext requestContext) - { - // Execute the given PowerShell script and send the response. - // Note that we aren't waiting for execution to complete here - // because the debugger could stop while the script executes. - Task executeTask = - editorSession.PowerShellContext - .ExecuteScriptAtPath(this.scriptPathToLaunch, this.arguments) - .ContinueWith( - async (t) => { - Logger.Write(LogLevel.Verbose, "Execution completed, terminating..."); - - await requestContext.SendEvent( - TerminatedEvent.Type, - null); - - // Stop the server - this.Stop(); - }); - - await requestContext.SendResult(null); - } - protected Task HandleDisconnectRequest( object disconnectParams, RequestContext requestContext) From 5e6f75545298b571c087cb493696dadf43685594 Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Tue, 1 Mar 2016 20:32:58 -0700 Subject: [PATCH 2/3] Improved code comments. --- .../Server/DebugAdapter.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs index 44273f94d..0392ad23b 100644 --- a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs +++ b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs @@ -106,7 +106,7 @@ protected async Task HandleConfigurationDoneRequest( RequestContext requestContext) { // Ensure that only the second message between launch and - // configurationDone - actually launches the script. + // configurationDone requests, actually launches the script. lock (syncLock) { if (!this.isLaunchRequestComplete) @@ -116,8 +116,8 @@ protected async Task HandleConfigurationDoneRequest( } // The order of debug protocol messages apparently isn't as guaranteed as we might like. - // Need to be able to handle the case where we get configurationDone after launch request - // and vice-versa. + // Need to be able to handle the case where we get the configurationDone request after the + // launch request. if (this.isLaunchRequestComplete) { this.LaunchScript(requestContext); @@ -162,13 +162,13 @@ protected async Task HandleLaunchRequest( // We may not actually launch the script in response to this // request unless it comes after the configurationDone request. // If the launch request comes first, then stash the launch - // params so that the subsequent configurationDone request can - // launch the script. + // params so that the subsequent configurationDone request handler + // can launch the script. this.scriptPathToLaunch = launchParams.Program; this.arguments = arguments; // Ensure that only the second message between launch and - // configurationDone - actually launches the script. + // configurationDone requests, actually launches the script. lock (syncLock) { if (!this.isConfigurationDoneRequestComplete) @@ -178,8 +178,8 @@ protected async Task HandleLaunchRequest( } // The order of debug protocol messages apparently isn't as guaranteed as we might like. - // Need to be able to handle the case where we get configurationDone after launch request - // and vice-versa. + // Need to be able to handle the case where we get the launch request after the + // configurationDone request. if (this.isConfigurationDoneRequestComplete) { this.LaunchScript(requestContext); From f1be4b631d5f88ef03ff610b76aa023841d82d79 Mon Sep 17 00:00:00 2001 From: Keith Hill Date: Wed, 2 Mar 2016 11:21:35 -0700 Subject: [PATCH 3/3] Got rid of lock object and locks. Rely upon serialized handling of messages. --- .../Server/DebugAdapter.cs | 25 +++---------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs index 0392ad23b..cfc7b192a 100644 --- a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs +++ b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs @@ -20,7 +20,6 @@ public class DebugAdapter : DebugAdapterBase { private EditorSession editorSession; private OutputDebouncer outputDebouncer; - private object syncLock = new object(); private bool isConfigurationDoneRequestComplete; private bool isLaunchRequestComplete; private string scriptPathToLaunch; @@ -105,16 +104,6 @@ protected async Task HandleConfigurationDoneRequest( object args, RequestContext requestContext) { - // Ensure that only the second message between launch and - // configurationDone requests, actually launches the script. - lock (syncLock) - { - if (!this.isLaunchRequestComplete) - { - this.isConfigurationDoneRequestComplete = true; - } - } - // The order of debug protocol messages apparently isn't as guaranteed as we might like. // Need to be able to handle the case where we get the configurationDone request after the // launch request. @@ -123,6 +112,8 @@ protected async Task HandleConfigurationDoneRequest( this.LaunchScript(requestContext); } + this.isConfigurationDoneRequestComplete = true; + await requestContext.SendResult(null); } @@ -167,16 +158,6 @@ protected async Task HandleLaunchRequest( this.scriptPathToLaunch = launchParams.Program; this.arguments = arguments; - // Ensure that only the second message between launch and - // configurationDone requests, actually launches the script. - lock (syncLock) - { - if (!this.isConfigurationDoneRequestComplete) - { - this.isLaunchRequestComplete = true; - } - } - // The order of debug protocol messages apparently isn't as guaranteed as we might like. // Need to be able to handle the case where we get the launch request after the // configurationDone request. @@ -185,6 +166,8 @@ protected async Task HandleLaunchRequest( this.LaunchScript(requestContext); } + this.isLaunchRequestComplete = true; + await requestContext.SendResult(null); }