Skip to content

Support -CustomPipeName #871

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 4 commits into from
Mar 10, 2019
Merged

Support -CustomPipeName #871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13e1385
Support -CustomPipeName
TylerLeonhardt Feb 26, 2019
5162fb9
codacy fix
TylerLeonhardt Mar 5, 2019
fc7b4ad
feedback from @rjmholt
TylerLeonhardt Mar 6, 2019
89d2217
address more feedback from @rjmholt
TylerLeonhardt Mar 7, 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 CustomPipeName { get; set; }
}
}
65 changes: 43 additions & 22 deletions src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ namespace Microsoft.PowerShell.EditorServices.Protocol.Server
{
public class DebugAdapter
{
private static readonly Version _minVersionForCustomPipeName = new Version(6, 2);

private EditorSession _editorSession;

private bool _noDebug;
Expand Down Expand Up @@ -350,11 +352,17 @@ protected async Task HandleAttachRequestAsync(

RegisterEventHandlers();

bool processIdIsSet = !string.IsNullOrEmpty(attachParams.ProcessId) && attachParams.ProcessId != "undefined";
bool customPipeNameIsSet = !string.IsNullOrEmpty(attachParams.CustomPipeName) && attachParams.CustomPipeName != "undefined";

PowerShellVersionDetails runspaceVersion =
_editorSession.PowerShellContext.CurrentRunspace.PowerShellVersion;

// 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 (string.IsNullOrEmpty(attachParams.ProcessId) || (attachParams.ProcessId == "undefined"))
if (!processIdIsSet && !customPipeNameIsSet)
{
Logger.Write(
LogLevel.Normal,
Expand All @@ -370,9 +378,6 @@ await requestContext.SendErrorAsync(

if (attachParams.ComputerName != null)
{
PowerShellVersionDetails runspaceVersion =
_editorSession.PowerShellContext.CurrentRunspace.PowerShellVersion;

if (runspaceVersion.Version.Major < 4)
{
await requestContext.SendErrorAsync(
Expand Down Expand Up @@ -403,16 +408,12 @@ await requestContext.SendErrorAsync(
_isRemoteAttach = true;
}

if (int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0))
if (processIdIsSet && int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0))
{
PowerShellVersionDetails runspaceVersion =
_editorSession.PowerShellContext.CurrentRunspace.PowerShellVersion;

if (runspaceVersion.Version.Major < 5)
{
await requestContext.SendErrorAsync(
$"Attaching to a process is only available with PowerShell 5 and higher (current session is {runspaceVersion.Version}).");

return;
}

Expand All @@ -427,20 +428,27 @@ await requestContext.SendErrorAsync(

return;
}
}
else if (customPipeNameIsSet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do all of the clauses above return? If so, is it possible to remove this else?

Copy link
Member Author

Choose a reason for hiding this comment

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

They don't all return. We have to keep this else in.

{
if (runspaceVersion.Version < _minVersionForCustomPipeName)
{
await requestContext.SendErrorAsync(
$"Attaching to a process with CustomPipeName is only available with PowerShell 6.2 and higher (current session is {runspaceVersion.Version}).");
return;
}

// Clear any existing breakpoints before proceeding
await ClearSessionBreakpointsAsync();

// Execute the Debug-Runspace command but don't await it because it
// 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;
_waitingForAttach = true;
Task nonAwaitedTask =
_editorSession.PowerShellContext
.ExecuteScriptStringAsync($"\nDebug-Runspace -Id {runspaceId}")
.ContinueWith(OnExecutionCompletedAsync);
await _editorSession.PowerShellContext.ExecuteScriptStringAsync(
$"Enter-PSHostProcess -CustomPipeName {attachParams.CustomPipeName}",
errorMessages);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you still get intellisense while in the attached process? I would think that this would hold up the request queue, or does Enter-PSHostProcess return in the original runspace after attaching?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is so little time between Enter-PSHostProcess and the Debug-Runspace call bellow that it doesn't really matter. Debug-Runspace will for sure block any intellisense call until a breakpoint is hit. Then once a breakpoint is hit, it will work again but from the context of the breakpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does that answer your question?

FWIW, I'm not changing that behavior in this PR. In the old version it still:

awaited Enter-PSHostProcess
not awaited Debug-Runspace

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked about this offline. We are good.


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

return;
}
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good opportunity to take this out?

Copy link
Member Author

@TylerLeonhardt TylerLeonhardt Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Given they additional check to make sure processId is an int, refactoring to remove else just doesn't seem as readable as this.

{
Expand All @@ -454,6 +462,19 @@ await requestContext.SendErrorAsync(
return;
}

// Clear any existing breakpoints before proceeding
await ClearSessionBreakpointsAsync().ConfigureAwait(continueOnCapturedContext: false);

// Execute the Debug-Runspace command but don't await it because it
// 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;
_waitingForAttach = true;
Task nonAwaitedTask = _editorSession.PowerShellContext
.ExecuteScriptStringAsync($"\nDebug-Runspace -Id {runspaceId}")
.ContinueWith(OnExecutionCompletedAsync);

await requestContext.SendResultAsync(null);
}

Expand Down