From 69d699bb8e20ddc45e88e0ae21d325928d695e7b Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 26 Feb 2020 20:32:54 -0800 Subject: [PATCH 1/6] Fix bad context state after debugging --- .../PowerShellContextService.cs | 56 +++++++++++-------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs index 8bf56b7a3..1d92c6c24 100644 --- a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs +++ b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs @@ -68,6 +68,7 @@ static PowerShellContextService() #region Fields private readonly SemaphoreSlim resumeRequestHandle = AsyncUtils.CreateSimpleLockingSemaphore(); + private readonly SemaphoreSlim sessionStateLock = AsyncUtils.CreateSimpleLockingSemaphore(); private readonly OmniSharp.Extensions.LanguageServer.Protocol.Server.ILanguageServer _languageServer; private readonly bool isPSReadLineEnabled; @@ -745,6 +746,7 @@ public async Task> ExecuteCommandAsync( // Don't change our SessionState for ReadLine. if (!executionOptions.IsReadLine) { + await this.sessionStateLock.WaitAsync(); shell.InvocationStateChanged += PowerShell_InvocationStateChanged; } @@ -768,6 +770,7 @@ public async Task> ExecuteCommandAsync( if (!executionOptions.IsReadLine) { shell.InvocationStateChanged -= PowerShell_InvocationStateChanged; + this.sessionStateLock.Release(); } if (shell.HadErrors) @@ -1204,45 +1207,52 @@ public void AbortExecution() /// public void AbortExecution(bool shouldAbortDebugSession) { - if (this.SessionState != PowerShellContextState.Aborting && - this.SessionState != PowerShellContextState.Disposed) + if (this.SessionState == PowerShellContextState.Aborting + || this.SessionState == PowerShellContextState.Disposed) { - this.logger.LogTrace("Execution abort requested..."); + this.logger.LogTrace( + string.Format( + $"Execution abort requested when already aborted (SessionState = {this.SessionState})")); + return; + } + + this.logger.LogTrace("Execution abort requested..."); + + if (shouldAbortDebugSession) + { + this.ExitAllNestedPrompts(); + } + if (this.PromptNest.IsInDebugger) + { if (shouldAbortDebugSession) { - this.ExitAllNestedPrompts(); - } - - if (this.PromptNest.IsInDebugger) - { - if (shouldAbortDebugSession) - { - this.versionSpecificOperations.StopCommandInDebugger(this); - this.ResumeDebugger(DebuggerResumeAction.Stop); - } - else - { - this.versionSpecificOperations.StopCommandInDebugger(this); - } + this.versionSpecificOperations.StopCommandInDebugger(this); + this.ResumeDebugger(DebuggerResumeAction.Stop); } else { - this.PromptNest.GetPowerShell(isReadLine: false).BeginStop(null, null); + this.versionSpecificOperations.StopCommandInDebugger(this); } + } + else + { + this.PromptNest.GetPowerShell(isReadLine: false).BeginStop(null, null); + } + this.sessionStateLock.Wait(); + try + { this.SessionState = PowerShellContextState.Aborting; - this.OnExecutionStatusChanged( ExecutionStatus.Aborted, null, false); } - else + finally { - this.logger.LogTrace( - string.Format( - $"Execution abort requested when already aborted (SessionState = {this.SessionState})")); + this.SessionState = PowerShellContextState.Ready; + this.sessionStateLock.Release(); } } From 6433f6d155b604cb23c9752fede63aa6ccee65b2 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 26 Feb 2020 20:38:49 -0800 Subject: [PATCH 2/6] Add TODO --- .../Services/PowerShellContext/PowerShellContextService.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs index 1d92c6c24..2a06ba56f 100644 --- a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs +++ b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs @@ -1240,6 +1240,10 @@ public void AbortExecution(bool shouldAbortDebugSession) this.PromptNest.GetPowerShell(isReadLine: false).BeginStop(null, null); } + // TODO: + // This lock and state reset are a temporary fix at best. + // We need to investigate how the debugger should be interacting + // with PowerShell in this cancellation scenario. this.sessionStateLock.Wait(); try { From 253f1af227e5d4f6157485bb56c9ac5773f76c1c Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 26 Feb 2020 20:40:24 -0800 Subject: [PATCH 3/6] Style fix --- .../Services/PowerShellContext/PowerShellContextService.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs index 2a06ba56f..1be1dcd57 100644 --- a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs +++ b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs @@ -1225,15 +1225,11 @@ public void AbortExecution(bool shouldAbortDebugSession) if (this.PromptNest.IsInDebugger) { + this.versionSpecificOperations.StopCommandInDebugger(this); if (shouldAbortDebugSession) { - this.versionSpecificOperations.StopCommandInDebugger(this); this.ResumeDebugger(DebuggerResumeAction.Stop); } - else - { - this.versionSpecificOperations.StopCommandInDebugger(this); - } } else { From b77617af453b69cf033f7b8f65dba3840a15fedd Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 27 Feb 2020 21:23:02 -0800 Subject: [PATCH 4/6] Only lock execution when it hasn't begun --- .../PowerShellContextService.cs | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs index 1be1dcd57..d4e1ef797 100644 --- a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs +++ b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs @@ -22,10 +22,8 @@ namespace Microsoft.PowerShell.EditorServices.Services { using System.Diagnostics.CodeAnalysis; using System.Management.Automation; - using System.Runtime.InteropServices; using Microsoft.PowerShell.EditorServices.Handlers; using Microsoft.PowerShell.EditorServices.Hosting; - using Microsoft.PowerShell.EditorServices.Logging; using Microsoft.PowerShell.EditorServices.Services.PowerShellContext; /// @@ -1240,19 +1238,21 @@ public void AbortExecution(bool shouldAbortDebugSession) // This lock and state reset are a temporary fix at best. // We need to investigate how the debugger should be interacting // with PowerShell in this cancellation scenario. - this.sessionStateLock.Wait(); - try + if (this.sessionStateLock.Wait(0)) { - this.SessionState = PowerShellContextState.Aborting; - this.OnExecutionStatusChanged( - ExecutionStatus.Aborted, - null, - false); - } - finally - { - this.SessionState = PowerShellContextState.Ready; - this.sessionStateLock.Release(); + try + { + this.SessionState = PowerShellContextState.Aborting; + this.OnExecutionStatusChanged( + ExecutionStatus.Aborted, + null, + false); + } + finally + { + this.SessionState = PowerShellContextState.Ready; + this.sessionStateLock.Release(); + } } } From 57552b3a1089657aa1b1254792c3a40a5b0a9383 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 27 Feb 2020 21:25:28 -0800 Subject: [PATCH 5/6] Add comment --- .../Services/PowerShellContext/PowerShellContextService.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs index d4e1ef797..c341ee873 100644 --- a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs +++ b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs @@ -1238,6 +1238,10 @@ public void AbortExecution(bool shouldAbortDebugSession) // This lock and state reset are a temporary fix at best. // We need to investigate how the debugger should be interacting // with PowerShell in this cancellation scenario. + // + // Currently we try to acquire a lock on the execution status changed event. + // If we can't, it's because a command is executing, so we shouldn't change the status. + // If we can, we own the status and should fire the event. if (this.sessionStateLock.Wait(0)) { try From 0548d416e7004fa86c36375eea678ee30706d20e Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 27 Feb 2020 21:26:16 -0800 Subject: [PATCH 6/6] Add the dreaded ConfigureAwait(false) --- .../Services/PowerShellContext/PowerShellContextService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs index c341ee873..4cde19911 100644 --- a/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs +++ b/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs @@ -744,7 +744,7 @@ public async Task> ExecuteCommandAsync( // Don't change our SessionState for ReadLine. if (!executionOptions.IsReadLine) { - await this.sessionStateLock.WaitAsync(); + await this.sessionStateLock.WaitAsync().ConfigureAwait(false); shell.InvocationStateChanged += PowerShell_InvocationStateChanged; }