From 2968e35b2978d247ee1451eceabd207e3f30598f Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Fri, 12 Jan 2024 17:07:17 -0800 Subject: [PATCH] Fix up debugger attach handlers First off, the error messages were never actually displayed to the user because the RpcErrorException constructor takes three arguments. Now the second argument is always (correctly but annoyingly) null. Secondly, we do not support attaching to PowerShell Editor Services. It sure looked like we did (because we had special logic for it) but once attached, nothing worked. So it was half-baked. Now we throw an error if the user is trying to do that. Thirdly, because of that half-baked implementation, the process ID field was typed as a string (to support "current" as a shortcut) but that caused a mess here and an error in the VS Code client. Now it's just always an integer. (Same for the runspace ID.) Fourthly, a big mess was cleaned up by refactoring using functions, who'd have thought? Fifth and finally, superfluous version checking around PowerShell <5.1 was removed (as those versions are no longer supported whatsoever). --- .../Handlers/DebuggerActionHandlers.cs | 2 +- .../Handlers/LaunchAndAttachHandler.cs | 253 ++++++++---------- .../Handlers/SetVariableHandler.cs | 16 +- .../Handlers/IGetRunspaceHandler.cs | 2 +- .../PSHostProcessAndRunspaceHandlers.cs | 110 ++++---- .../LanguageServerProtocolMessageTests.cs | 2 +- 6 files changed, 180 insertions(+), 205 deletions(-) diff --git a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/DebuggerActionHandlers.cs b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/DebuggerActionHandlers.cs index 2276aee750..1e830a2c19 100644 --- a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/DebuggerActionHandlers.cs +++ b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/DebuggerActionHandlers.cs @@ -51,7 +51,7 @@ public override Task Handle(PauseArguments request, CancellationT } catch (NotSupportedException e) { - throw new RpcErrorException(0, e.Message); + throw new RpcErrorException(0, null, e.Message); } } } diff --git a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs index ac6490c7b5..de40648536 100644 --- a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs +++ b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs @@ -76,9 +76,9 @@ internal record PsesAttachRequestArguments : AttachRequestArguments { public string ComputerName { get; set; } - public string ProcessId { get; set; } + public int ProcessId { get; set; } - public string RunspaceId { get; set; } + public int RunspaceId { get; set; } public string RunspaceName { get; set; } @@ -87,6 +87,7 @@ internal record PsesAttachRequestArguments : AttachRequestArguments internal class LaunchAndAttachHandler : ILaunchHandler, IAttachHandler, IOnDebugAdapterServerStarted { + private static readonly int currentProcessId = System.Diagnostics.Process.GetCurrentProcess().Id; private static readonly Version s_minVersionForCustomPipeName = new(6, 2); private readonly ILogger _logger; private readonly BreakpointService _breakpointService; @@ -190,7 +191,7 @@ public async Task Handle(PsesLaunchRequestArguments request, Can && !string.IsNullOrEmpty(request.Script) && ScriptFile.IsUntitledPath(request.Script)) { - throw new RpcErrorException(0, "Running an Untitled file in a temporary Extension Terminal is currently not supported."); + throw new RpcErrorException(0, null, "Running an Untitled file in a temporary Extension Terminal is currently not supported!"); } // If the current session is remote, map the script path to the remote @@ -239,59 +240,26 @@ private async Task HandleImpl(PsesAttachRequestArguments request { // The debugger has officially started. We use this to later check if we should stop it. ((PsesInternalHost)_executionService).DebugContext.IsActive = true; - _debugStateService.IsAttachSession = true; - _debugEventHandlerService.RegisterEventHandlers(); - bool processIdIsSet = !string.IsNullOrEmpty(request.ProcessId) && request.ProcessId != "undefined"; + bool processIdIsSet = request.ProcessId != 0; bool customPipeNameIsSet = !string.IsNullOrEmpty(request.CustomPipeName) && request.CustomPipeName != "undefined"; - PowerShellVersionDetails runspaceVersion = _runspaceContext.CurrentRunspace.PowerShellVersionDetails; - // If there are no host processes to attach to or the user cancels selection, we get a null for the process id. // This is not an error, just a request to stop the original "attach to" request. // Testing against "undefined" is a HACK because I don't know how to make "Cancel" on quick pick loading // to cancel on the VSCode side without sending an attachRequest with processId set to "undefined". if (!processIdIsSet && !customPipeNameIsSet) { - _logger.LogInformation( - $"Attach request aborted, received {request.ProcessId} for processId."); - - throw new RpcErrorException(0, "User aborted attach to PowerShell host process."); + string msg = $"User aborted attach to PowerShell host process: {request.ProcessId}"; + _logger.LogTrace(msg); + throw new RpcErrorException(0, null, msg); } - if (request.ComputerName != null) + if (!string.IsNullOrEmpty(request.ComputerName)) { - if (runspaceVersion.Version.Major < 4) - { - throw new RpcErrorException(0, $"Remote sessions are only available with PowerShell 4 and higher (current session is {runspaceVersion.Version})."); - } - else if (_runspaceContext.CurrentRunspace.RunspaceOrigin != RunspaceOrigin.Local) - { - throw new RpcErrorException(0, "Cannot attach to a process in a remote session when already in a remote session."); - } - - PSCommand enterPSSessionCommand = new PSCommand() - .AddCommand("Enter-PSSession") - .AddParameter("ComputerName", request.ComputerName); - - try - { - await _executionService.ExecutePSCommandAsync( - enterPSSessionCommand, - cancellationToken, - PowerShellExecutionOptions.ImmediateInteractive) - .ConfigureAwait(false); - } - catch (Exception e) - { - string msg = $"Could not establish remote session to computer '{request.ComputerName}'"; - _logger.LogError(e, msg); - throw new RpcErrorException(0, msg); - } - - _debugStateService.IsRemoteAttach = true; + await AttachToComputer(request.ComputerName, cancellationToken).ConfigureAwait(false); } // Set up a temporary runspace changed event handler so we can ensure @@ -305,131 +273,62 @@ void RunspaceChangedHandler(object s, RunspaceChangedEventArgs _) runspaceChanged.TrySetResult(true); } - _executionService.RunspaceChanged += RunspaceChangedHandler; - - if (processIdIsSet && int.TryParse(request.ProcessId, out int processId) && (processId > 0)) + if (processIdIsSet) { - if (runspaceVersion.Version.Major < 5) + if (request.ProcessId == currentProcessId) { - throw new RpcErrorException(0, $"Attaching to a process is only available with PowerShell 5 and higher (current session is {runspaceVersion.Version})."); + throw new RpcErrorException(0, null, $"Attaching to the Extension Terminal is not supported!"); } - PSCommand enterPSHostProcessCommand = new PSCommand() - .AddCommand("Enter-PSHostProcess") - .AddParameter("Id", processId); - - try - { - await _executionService.ExecutePSCommandAsync( - enterPSHostProcessCommand, - cancellationToken, - PowerShellExecutionOptions.ImmediateInteractive) - .ConfigureAwait(false); - } - catch (Exception e) - { - string msg = $"Could not attach to process with Id: '{request.ProcessId}'"; - _logger.LogError(e, msg); - throw new RpcErrorException(0, msg); - } + _executionService.RunspaceChanged += RunspaceChangedHandler; + await AttachToProcess(request.ProcessId, cancellationToken).ConfigureAwait(false); + await runspaceChanged.Task.ConfigureAwait(false); } else if (customPipeNameIsSet) { - if (runspaceVersion.Version < s_minVersionForCustomPipeName) - { - throw new RpcErrorException(0, $"Attaching to a process with CustomPipeName is only available with PowerShell 6.2 and higher (current session is {runspaceVersion.Version})."); - } - - PSCommand enterPSHostProcessCommand = new PSCommand() - .AddCommand("Enter-PSHostProcess") - .AddParameter("CustomPipeName", request.CustomPipeName); - - try - { - await _executionService.ExecutePSCommandAsync( - enterPSHostProcessCommand, - cancellationToken, - PowerShellExecutionOptions.ImmediateInteractive) - .ConfigureAwait(false); - } - catch (Exception e) - { - string msg = $"Could not attach to process with CustomPipeName: '{request.CustomPipeName}'"; - _logger.LogError(e, msg); - throw new RpcErrorException(0, msg); - } + _executionService.RunspaceChanged += RunspaceChangedHandler; + await AttachToPipe(request.CustomPipeName, cancellationToken).ConfigureAwait(false); + await runspaceChanged.Task.ConfigureAwait(false); } - else if (request.ProcessId != "current") + else { - _logger.LogError( - $"Attach request failed, '{request.ProcessId}' is an invalid value for the processId."); - - throw new RpcErrorException(0, "A positive integer must be specified for the processId field."); + throw new RpcErrorException(0, null, "Invalid configuration with no process ID nor custom pipe name!"); } - await runspaceChanged.Task.ConfigureAwait(false); // Execute the Debug-Runspace command but don't await it because it - // will block the debug adapter initialization process. The + // will block the debug adapter initialization process. The // InitializedEvent will be sent as soon as the RunspaceChanged // event gets fired with the attached runspace. - PSCommand debugRunspaceCmd = new PSCommand().AddCommand("Debug-Runspace"); - if (request.RunspaceName != null) + if (!string.IsNullOrEmpty(request.RunspaceName)) { - PSCommand getRunspaceIdCommand = new PSCommand() + PSCommand psCommand = new PSCommand() .AddCommand(@"Microsoft.PowerShell.Utility\Get-Runspace") .AddParameter("Name", request.RunspaceName) .AddCommand(@"Microsoft.PowerShell.Utility\Select-Object") .AddParameter("ExpandProperty", "Id"); - try - { - IEnumerable ids = await _executionService.ExecutePSCommandAsync( - getRunspaceIdCommand, - cancellationToken) - .ConfigureAwait(false); - - foreach (int? id in ids) - { - _debugStateService.RunspaceId = id; - break; + IReadOnlyList results = await _executionService.ExecutePSCommandAsync(psCommand, cancellationToken).ConfigureAwait(false); - // TODO: If we don't end up setting this, we should throw - } - } - catch (Exception getRunspaceException) + if (results.Count == 0) { - _logger.LogError( - getRunspaceException, - "Unable to determine runspace to attach to. Message: {message}", - getRunspaceException.Message); + throw new RpcErrorException(0, null, $"Could not find ID of runspace: {request.RunspaceName}"); } - // TODO: We have the ID, why not just use that? - debugRunspaceCmd.AddParameter("Name", request.RunspaceName); + // Translate the runspace name to the runspace ID. + request.RunspaceId = results[0]; } - else if (request.RunspaceId != null) - { - if (!int.TryParse(request.RunspaceId, out int runspaceId) || runspaceId <= 0) - { - _logger.LogError( - $"Attach request failed, '{request.RunspaceId}' is an invalid value for the processId."); - - throw new RpcErrorException(0, "A positive integer must be specified for the RunspaceId field."); - } - - _debugStateService.RunspaceId = runspaceId; - debugRunspaceCmd.AddParameter("Id", runspaceId); - } - else + if (request.RunspaceId < 1) { - _debugStateService.RunspaceId = 1; - debugRunspaceCmd.AddParameter("Id", 1); + throw new RpcErrorException(0, null, "A positive integer must be specified for the RunspaceId!"); } + _debugStateService.RunspaceId = request.RunspaceId; + debugRunspaceCmd.AddParameter("Id", request.RunspaceId); + // Clear any existing breakpoints before proceeding await _breakpointService.RemoveAllBreakpointsAsync().ConfigureAwait(continueOnCapturedContext: false); @@ -438,11 +337,89 @@ await _executionService.ExecutePSCommandAsync( .ExecutePSCommandAsync(debugRunspaceCmd, CancellationToken.None, PowerShellExecutionOptions.ImmediateInteractive) .ContinueWith(OnExecutionCompletedAsync, TaskScheduler.Default); - if (runspaceVersion.Version.Major >= 7) + _debugStateService.ServerStarted.TrySetResult(true); + + return new AttachResponse(); + } + + private async Task AttachToComputer(string computerName, CancellationToken cancellationToken) + { + _debugStateService.IsRemoteAttach = true; + + if (_runspaceContext.CurrentRunspace.RunspaceOrigin != RunspaceOrigin.Local) { - _debugStateService.ServerStarted.TrySetResult(true); + throw new RpcErrorException(0, null, "Cannot attach to a process in a remote session when already in a remote session!"); + } + + PSCommand psCommand = new PSCommand() + .AddCommand("Enter-PSSession") + .AddParameter("ComputerName", computerName); + + try + { + await _executionService.ExecutePSCommandAsync( + psCommand, + cancellationToken, + PowerShellExecutionOptions.ImmediateInteractive) + .ConfigureAwait(false); + } + catch (Exception e) + { + string msg = $"Could not establish remote session to computer: {computerName}"; + _logger.LogError(e, msg); + throw new RpcErrorException(0, null, msg); + } + } + + private async Task AttachToProcess(int processId, CancellationToken cancellationToken) + { + PSCommand enterPSHostProcessCommand = new PSCommand() + .AddCommand(@"Microsoft.PowerShell.Core\Enter-PSHostProcess") + .AddParameter("Id", processId); + + try + { + await _executionService.ExecutePSCommandAsync( + enterPSHostProcessCommand, + cancellationToken, + PowerShellExecutionOptions.ImmediateInteractive) + .ConfigureAwait(false); + } + catch (Exception e) + { + string msg = $"Could not attach to process with ID: {processId}"; + _logger.LogError(e, msg); + throw new RpcErrorException(0, null, msg); + } + } + + private async Task AttachToPipe(string pipeName, CancellationToken cancellationToken) + { + PowerShellVersionDetails runspaceVersion = _runspaceContext.CurrentRunspace.PowerShellVersionDetails; + + if (runspaceVersion.Version < s_minVersionForCustomPipeName) + { + throw new RpcErrorException(0, null, $"Attaching to a process with CustomPipeName is only available with PowerShell 6.2 and higher. Current session is: {runspaceVersion.Version}"); + } + + PSCommand enterPSHostProcessCommand = new PSCommand() + .AddCommand(@"Microsoft.PowerShell.Core\Enter-PSHostProcess") + .AddParameter("CustomPipeName", pipeName); + + try + { + await _executionService.ExecutePSCommandAsync( + enterPSHostProcessCommand, + cancellationToken, + PowerShellExecutionOptions.ImmediateInteractive) + .ConfigureAwait(false); + } + catch (Exception e) + { + string msg = $"Could not attach to process with CustomPipeName: {pipeName}"; + _logger.LogError(e, msg); + throw new RpcErrorException(0, null, msg); } - return new AttachResponse(); } // PSES follows the following flow: diff --git a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/SetVariableHandler.cs b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/SetVariableHandler.cs index fdd9b7889d..75005dd853 100644 --- a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/SetVariableHandler.cs +++ b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/SetVariableHandler.cs @@ -38,20 +38,20 @@ await _debugService.SetVariableAsync( return new SetVariableResponse { Value = updatedValue }; } - catch (Exception ex) when (ex is ArgumentTransformationMetadataException or + catch (Exception e) when (e is ArgumentTransformationMetadataException or InvalidPowerShellExpressionException or SessionStateUnauthorizedAccessException) { // Catch common, innocuous errors caused by the user supplying a value that can't be converted or the variable is not settable. - _logger.LogTrace($"Failed to set variable: {ex.Message}"); - throw new RpcErrorException(0, ex.Message); + string msg = $"Failed to set variable: {e.Message}"; + _logger.LogTrace(msg); + throw new RpcErrorException(0, null, msg); } - catch (Exception ex) + catch (Exception e) { - _logger.LogError($"Unexpected error setting variable: {ex.Message}"); - string msg = - $"Unexpected error: {ex.GetType().Name} - {ex.Message} Please report this error to the PowerShellEditorServices project on GitHub."; - throw new RpcErrorException(0, msg); + string msg = $"Unexpected error setting variable: {e.Message}"; + _logger.LogError(msg); + throw new RpcErrorException(0, null, msg); } } } diff --git a/src/PowerShellEditorServices/Services/PowerShell/Handlers/IGetRunspaceHandler.cs b/src/PowerShellEditorServices/Services/PowerShell/Handlers/IGetRunspaceHandler.cs index 7cf86e45e6..7ca2c86a4a 100644 --- a/src/PowerShellEditorServices/Services/PowerShell/Handlers/IGetRunspaceHandler.cs +++ b/src/PowerShellEditorServices/Services/PowerShell/Handlers/IGetRunspaceHandler.cs @@ -11,7 +11,7 @@ internal interface IGetRunspaceHandler : IJsonRpcRequestHandler { - public string ProcessId { get; set; } + public int ProcessId { get; set; } } internal class RunspaceResponse diff --git a/src/PowerShellEditorServices/Services/PowerShell/Handlers/PSHostProcessAndRunspaceHandlers.cs b/src/PowerShellEditorServices/Services/PowerShell/Handlers/PSHostProcessAndRunspaceHandlers.cs index bff586d44c..fb86e4950c 100644 --- a/src/PowerShellEditorServices/Services/PowerShell/Handlers/PSHostProcessAndRunspaceHandlers.cs +++ b/src/PowerShellEditorServices/Services/PowerShell/Handlers/PSHostProcessAndRunspaceHandlers.cs @@ -11,92 +11,90 @@ namespace Microsoft.PowerShell.EditorServices.Handlers { using System.Management.Automation; using Microsoft.PowerShell.EditorServices.Services.PowerShell; + using Microsoft.PowerShell.EditorServices.Services.PowerShell.Runspace; + using OmniSharp.Extensions.JsonRpc; internal class PSHostProcessAndRunspaceHandlers : IGetPSHostProcessesHandler, IGetRunspaceHandler { private readonly ILogger _logger; private readonly IInternalPowerShellExecutionService _executionService; + private readonly IRunspaceContext _runspaceContext; + private static readonly int currentProcessId = System.Diagnostics.Process.GetCurrentProcess().Id; - public PSHostProcessAndRunspaceHandlers(ILoggerFactory factory, IInternalPowerShellExecutionService executionService) + public PSHostProcessAndRunspaceHandlers( + ILoggerFactory factory, + IInternalPowerShellExecutionService executionService, + IRunspaceContext runspaceContext) { _logger = factory.CreateLogger(); _executionService = executionService; + _runspaceContext = runspaceContext; } - public Task Handle(GetPSHostProcessesParams request, CancellationToken cancellationToken) + public async Task Handle(GetPSHostProcessesParams request, CancellationToken cancellationToken) { - List psHostProcesses = new(); + PSCommand psCommand = new PSCommand().AddCommand(@"Microsoft.PowerShell.Core\Get-PSHostProcessInfo"); + IReadOnlyList processes = await _executionService.ExecutePSCommandAsync( + psCommand, cancellationToken).ConfigureAwait(false); - int processId = System.Diagnostics.Process.GetCurrentProcess().Id; - - using (PowerShell pwsh = PowerShell.Create()) + List psHostProcesses = []; + foreach (dynamic p in processes) { - pwsh.AddCommand("Get-PSHostProcessInfo") - .AddCommand("Where-Object") - .AddParameter("Property", "ProcessId") - .AddParameter("NE") - .AddParameter("Value", processId.ToString()); - - System.Collections.ObjectModel.Collection processes = pwsh.Invoke(); - - if (processes != null) + PSHostProcessResponse response = new() { - foreach (dynamic p in processes) - { - psHostProcesses.Add( - new PSHostProcessResponse - { - ProcessName = p.ProcessName, - ProcessId = p.ProcessId, - AppDomainName = p.AppDomainName, - MainWindowTitle = p.MainWindowTitle - }); - } + ProcessName = p.ProcessName, + ProcessId = p.ProcessId, + AppDomainName = p.AppDomainName, + MainWindowTitle = p.MainWindowTitle + }; + + // NOTE: We do not currently support attaching to ourself in this manner, so we + // exclude our process. When we maybe eventually do, we should name it. + if (response.ProcessId == currentProcessId) + { + continue; } + + psHostProcesses.Add(response); } - return Task.FromResult(psHostProcesses.ToArray()); + return psHostProcesses.ToArray(); } public async Task Handle(GetRunspaceParams request, CancellationToken cancellationToken) { - IEnumerable runspaces = null; - - request.ProcessId ??= "current"; - - // If the processId is a valid int, we need to run Get-Runspace within that process - // otherwise just use the current runspace. - if (int.TryParse(request.ProcessId, out int pid)) + if (request.ProcessId == currentProcessId) { - // Create a remote runspace that we will invoke Get-Runspace in. - using Runspace rs = RunspaceFactory.CreateRunspace(new NamedPipeConnectionInfo(pid)); - using PowerShell ps = PowerShell.Create(); - rs.Open(); - ps.Runspace = rs; - // Returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later. - runspaces = ps.AddCommand(@"Microsoft.PowerShell.Utility\Get-Runspace").Invoke(); + throw new RpcErrorException(0, null, $"Attaching to the Extension Terminal is not supported!"); } - else + + // Create a remote runspace that we will invoke Get-Runspace in. + IReadOnlyList runspaces = []; + using (Runspace runspace = RunspaceFactory.CreateRunspace(new NamedPipeConnectionInfo(request.ProcessId))) { - PSCommand psCommand = new PSCommand().AddCommand(@"Microsoft.PowerShell.Utility\Get-Runspace"); - // returns (not deserialized) Runspaces. For simpler code, we use PSObject and rely on dynamic later. - runspaces = await _executionService.ExecutePSCommandAsync(psCommand, cancellationToken).ConfigureAwait(false); + using PowerShell pwsh = PowerShell.Create(); + runspace.Open(); + pwsh.Runspace = runspace; + // Returns deserialized Runspaces. For simpler code, we use PSObject and rely on dynamic later. + runspaces = pwsh.AddCommand(@"Microsoft.PowerShell.Utility\Get-Runspace").Invoke(); } - List runspaceResponses = new(); - - if (runspaces != null) + List runspaceResponses = []; + foreach (dynamic runspace in runspaces) { - foreach (dynamic runspace in runspaces) + // This is the special runspace used for debugging, we can't attach to it. + if (runspace.Name == "PSAttachRunspace") { - runspaceResponses.Add( - new RunspaceResponse - { - Id = runspace.Id, - Name = runspace.Name, - Availability = runspace.RunspaceAvailability.ToString() - }); + continue; } + + runspaceResponses.Add( + new RunspaceResponse + { + Id = runspace.Id, + Name = runspace.Name, + Availability = runspace.RunspaceAvailability.ToString() + }); } return runspaceResponses.ToArray(); diff --git a/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs b/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs index cfbc5daf72..b04679e126 100644 --- a/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs +++ b/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs @@ -601,7 +601,7 @@ await PsesLanguageClient "powerShell/getRunspace", new GetRunspaceParams { - ProcessId = $"{process.Id}" + ProcessId = process.Id }) .Returning(CancellationToken.None); }