Skip to content
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.

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

Fix up debugger configuration resolvers #4888

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Jan 13, 2024

Wow this was a mess. Needs PowerShell/PowerShellEditorServices#2130, and I'm a little worried about breaking changes but supplying "current" to processId is special-cased to still work (well, throw an appropriate warning), since the launch configuration types only generate warnings.

Ok so we removed the notion of IDs (which are integers) being strings because that broke serialization with recent updates. But on inspection we realized the whole point was to be able to say "current" and have the attach configuration attach to the currently running Extension Terminal. Except that this was an unsupported, half-baked scenario (sure, it attached, but it couldn't actually be debugged properly). So now that throws warnings in both the client and server. The defaults for processId and runspaceId where changed to null (again, now they're just integers) which means to query for them. The supplied but half-baked configuration was removed.

Fixes #4843

@andyleejordan andyleejordan force-pushed the andyleejordan/fixup-debug-configs branch from 5dcc6d7 to e9cb397 Compare January 17, 2024 01:35
@andyleejordan andyleejordan changed the title WIP: Fix up debug config resolution stuff Fix up debugger configuration resolvers Jan 17, 2024
@andyleejordan andyleejordan marked this pull request as ready for review January 17, 2024 01:36
@andyleejordan andyleejordan requested review from a team and JustinGrote January 17, 2024 01:36
Copy link
Collaborator

@JustinGrote JustinGrote left a comment

Choose a reason for hiding this comment

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

LGTM

@andyleejordan andyleejordan force-pushed the andyleejordan/fixup-debug-configs branch from e9cb397 to 5678505 Compare January 22, 2024 18:33
@JustinGrote
Copy link
Collaborator

Still happening to me in a code --profile-temp with the extension installed from vsix. I checked the source zip and the JS looks to match the PR

  async pickPSHostProcess() {
    await this.getLanguageClient();
    const items = [];
    const response = await this.languageClient?.sendRequest(GetPSHostProcessesRequestType, {});
    for (const process3 of response ?? []) {
      let windowTitle = "";
      if (process3.mainWindowTitle) {
        windowTitle = `, ${process3.mainWindowTitle}`;
      }
      items.push({
        label: process3.processName,
        description: `PID: ${process3.processId.toString()}${windowTitle}`,
        processId: process3.processId
      });
    }

Let me try it in extension debug

Copy link
Member Author

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

From Code Review

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/features/DebugSession.ts Outdated Show resolved Hide resolved
src/features/DebugSession.ts Show resolved Hide resolved
@andyleejordan
Copy link
Member Author

Now also resolves the root of #4699.

@JustinGrote JustinGrote self-requested a review January 24, 2024 00:24
@andyleejordan andyleejordan force-pushed the andyleejordan/fixup-debug-configs branch from eb87b2e to e1b63cc Compare January 24, 2024 18:39
Ok so we removed the notion of IDs (which are integers) being strings
because that broke serialization with recent updates. But on inspection
we realized the whole point was to be able to say "current" and have the
attach configuration attach to the currently running Extension Terminal.
Except that this was an unsupported, half-baked scenario (sure, it
attached, but it couldn't actually be debugged properly). So now that
throws warnings in both the client and server. The defaults for
`processId` and `runspaceId` where changed to `null` (again, now they're
just integers) which means to query for them. The supplied but
half-baked configuration was removed.
This makes a lot more sense and fixes a long-standing bug.  Also cleaned up a
lot while at it.

1. `SessionManager` is created and the features are injected into the session
manager with the intention of them being able to "register" their language
client event handlers in the period between when the client is created and when
it is started.

2. For each feature, if it gets to a point where it needs the language client,
we want them to wait on a promise for the "started" client to be provided from
the session manager before continuing.

3. Once it is started, the session manager passes the client to
`LanguageClientConsumer` which resolves the promise for the features so they can
continue.
@andyleejordan andyleejordan force-pushed the andyleejordan/fixup-debug-configs branch from e1b63cc to d429beb Compare January 24, 2024 21:04
@andyleejordan andyleejordan added this pull request to the merge queue Jan 24, 2024
Merged via the queue into main with commit 0dceec3 Jan 24, 2024
7 checks passed
@andyleejordan andyleejordan deleted the andyleejordan/fixup-debug-configs branch January 24, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Cannot attach debugger to PS 7.4.0 processes
2 participants