Skip to content

Add attach to local runspace. #875

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.

Sign up for GitHub

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
merged 11 commits into from
Mar 13, 2019
Merged

Add attach to local runspace. #875

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d1ca49a
Add attach to local runspace.
adamdriscoll Mar 7, 2019
c794be4
Remove comment.
adamdriscoll Mar 7, 2019
384b91a
Use single runspaceId property.
adamdriscoll Mar 9, 2019
75220a6
Merge changes for custom pipe name.
adamdriscoll Mar 11, 2019
70a2e12
Remove unnecessary processId check.
adamdriscoll Mar 11, 2019
cbe5b18
Factored if.
adamdriscoll Mar 11, 2019
28f3840
Update src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs
TylerLeonhardt Mar 11, 2019
1d79002
Use runspace rather than dynamic.
adamdriscoll Mar 11, 2019
6466c44
Merge branch 'runspace-attach' of https://github.com/adamdriscoll/Pow…
adamdriscoll Mar 11, 2019
015c1ef
Accept process ID for GetRunspaceRequest.
adamdriscoll Mar 12, 2019
bb984a0
Clean up.
adamdriscoll Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/PowerShellEditorServices.Protocol/DebugAdapter/AttachRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,7 @@ public class AttachRequestArguments
public string ProcessId { get; set; }

public int RunspaceId { get; set; }

public string LocalRunspaceId { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking but ... do we really need two different fields for runspace id?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW if it would be more convenient if RunspaceId was a string, we change the type I think. We'd just update the VSCode debug attach code to stringify the runspace id number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I can make that change. That's pretty much the reason I added another field was that I didn't wanna mess with RunspaceId's type but I'm happy to adjust that to eliminate the new property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was curious how this affects existing launch.json files. Is there something we need to do to ensure that they get upgraded in some way or will a number just work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried this. Seems to still work. It complains in the launch.json that the number should be a string but the attaching still works.

}
}
25 changes: 25 additions & 0 deletions src/PowerShellEditorServices.Protocol/LanguageServer/GetRunspaceRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
//

using Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol;

namespace Microsoft.PowerShell.EditorServices.Protocol.LanguageServer
{
public class GetRunspaceRequest
{
public static readonly
RequestType<object, GetRunspaceResponse[], object, object> Type =
RequestType<object, GetRunspaceResponse[], object, object>.Create("powerShell/getRunspace");
}

public class GetRunspaceResponse
{
public int Id { get; set; }

public string Name { get; set; }

public string Availability { get; set; }
}
}
39 changes: 31 additions & 8 deletions src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ await requestContext.SendErrorAsync(
_isRemoteAttach = true;
}

if (int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0))
if ((int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0)) || attachParams.ProcessId == "current")
{
PowerShellVersionDetails runspaceVersion =
_editorSession.PowerShellContext.CurrentRunspace.PowerShellVersion;
Expand All @@ -416,16 +416,18 @@ await requestContext.SendErrorAsync(
return;
}

await _editorSession.PowerShellContext.ExecuteScriptStringAsync(
if (attachParams.ProcessId != "current") {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need this if because the outer if tries to parse the ProcessId as an integer (L411):

if (processIdIsSet && int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated the PR.

await _editorSession.PowerShellContext.ExecuteScriptStringAsync(
$"Enter-PSHostProcess -Id {processId}",
errorMessages);

if (errorMessages.Length > 0)
{
await requestContext.SendErrorAsync(
$"Could not attach to process '{processId}'");
if (errorMessages.Length > 0)
{
await requestContext.SendErrorAsync(
$"Could not attach to process '{processId}'");

return;
return;
}
}

// Clear any existing breakpoints before proceeding
Expand All @@ -435,7 +437,28 @@ await requestContext.SendErrorAsync(
// will block the debug adapter initialization process. The
// InitializedEvent will be sent as soon as the RunspaceChanged
// event gets fired with the attached runspace.
int runspaceId = attachParams.RunspaceId > 0 ? attachParams.RunspaceId : 1;

int runspaceId = 1;
if (string.IsNullOrEmpty(attachParams.LocalRunspaceId))
{
runspaceId = attachParams.RunspaceId > 0 ? attachParams.RunspaceId : 1;
}
else if (int.TryParse(attachParams.LocalRunspaceId, out int localRunspaceId))
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we could int.TryParse() on a string typed RunspaceId. In the normal "attach" case we'd just set it to "1". Your new debug config would set it to whatever the user picks from the runspace id pick list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet. Works for me. I will update these PRs sometime this weekend.

{
runspaceId = localRunspaceId;
}
else
{
Logger.Write(
LogLevel.Error,
$"Attach request failed, '{attachParams.LocalRunspaceId}' is an invalid value for the processId.");

await requestContext.SendErrorAsync(
"A positive integer must be specified for the LocalRunspaceId field.");

return;
}

_waitingForAttach = true;
Task nonAwaitedTask =
_editorSession.PowerShellContext
Expand Down
33 changes: 33 additions & 0 deletions src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using System.Linq;
using System.Management.Automation;
using System.Management.Automation.Language;
using System.Management.Automation.Runspaces;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
Expand Down Expand Up @@ -161,6 +162,8 @@ public void Start()
this.messageHandlers.SetRequestHandler(GetPSHostProcessesRequest.Type, this.HandleGetPSHostProcessesRequestAsync);
this.messageHandlers.SetRequestHandler(CommentHelpRequest.Type, this.HandleCommentHelpRequestAsync);

this.messageHandlers.SetRequestHandler(GetRunspaceRequest.Type, this.HandleGetRunspaceRequestAsync);

// Initialize the extension service
// TODO: This should be made awaited once Initialize is async!
this.editorSession.ExtensionService.InitializeAsync(
Expand Down Expand Up @@ -1218,6 +1221,36 @@ protected async Task HandleCommentHelpRequestAsync(
await requestContext.SendResultAsync(result);
}

protected async Task HandleGetRunspaceRequestAsync(
object noParams,
RequestContext<GetRunspaceResponse[]> requestContext)
{
var runspaceResponses = new List<GetRunspaceResponse>();

if (this.editorSession.PowerShellContext.LocalPowerShellVersion.Version.Major >= 5)
Copy link
Member

Choose a reason for hiding this comment

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

Was Get-Runspace added in 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good question. I just took that check from the process picker but I'm not sure when Debug-Runspace\Get-Runspace was added. I can try and find out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Well looks like that answers our question :)

{
var psCommand = new PSCommand();
psCommand.AddCommand("Get-Runspace");

var runspaces = await editorSession.PowerShellContext.ExecuteCommandAsync<PSObject>(psCommand);
Copy link
Member

Choose a reason for hiding this comment

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

can you replace var with the type since this is a tad more complicated than new Blah()

Copy link
Member

Choose a reason for hiding this comment

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

Also, instead of PSObject you should be able to do System.Management.Automation.Runspaces.Runspace and then you don't have to use dynamic below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

if (runspaces != null)
{
foreach (dynamic p in runspaces)
{
runspaceResponses.Add(
new GetRunspaceResponse
{
Id = p.Id,
Name = p.Name,
Availability = p.RunspaceAvailability.ToString()
});
}
}
}

await requestContext.SendResultAsync(runspaceResponses.ToArray());
}

private bool IsQueryMatch(string query, string symbolName)
{
return symbolName.IndexOf(query, StringComparison.OrdinalIgnoreCase) >= 0;
Expand Down