Skip to content

Remove TS debug adapter subprocess support #12008

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
karthiknadig merged 4 commits into from
May 27, 2020
Merged

Remove TS debug adapter subprocess support #12008

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
db983f1
Remove TS debug adapter subprocess support
karthiknadig May 27, 2020
b409ddf
Revert "Update pinned node version"
karthiknadig May 27, 2020
d50d433
Address comments
karthiknadig May 27, 2020
575afa8
Fix the node version merge issue
karthiknadig May 27, 2020
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
8 changes: 3 additions & 5 deletions src/client/debugger/extension/hooks/childProcessAttachHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { DebugConfiguration, DebugSessionCustomEvent } from 'vscode';
import { swallowExceptions } from '../../../common/utils/decorators';
import { AttachRequestArguments } from '../../types';
import { DebuggerEvents } from './constants';
import { ChildProcessLaunchData, IChildProcessAttachService, IDebugSessionEventHandlers } from './types';
import { IChildProcessAttachService, IDebugSessionEventHandlers } from './types';

/**
* This class is responsible for automatically attaching the debugger to any
Expand All @@ -29,10 +29,8 @@ export class ChildProcessAttachEventHandler implements IDebugSessionEventHandler
return;
}

let data: ChildProcessLaunchData | (AttachRequestArguments & DebugConfiguration);
if (event.event === DebuggerEvents.ChildProcessLaunched) {
data = event.body! as ChildProcessLaunchData;
} else if (
let data: AttachRequestArguments & DebugConfiguration;
if (
event.event === DebuggerEvents.PtvsdAttachToSubprocess ||
event.event === DebuggerEvents.DebugpyAttachToSubprocess
) {
Comment on lines 34 to 36
Copy link
Copy Markdown

@ericsnowcurrently ericsnowcurrently May 27, 2020

Choose a reason for hiding this comment

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

With a single case we can simplify:

        if (
            event.event !== DebuggerEvents.PtvsdAttachToSubprocess &&
            event.event !== DebuggerEvents.DebugpyAttachToSubprocess
        ) {
            return;
        }
        const data = event.body! as AttachRequestArguments & DebugConfiguration;
        if (Object.keys(data).length > 0) {

Expand Down
63 changes: 6 additions & 57 deletions src/client/debugger/extension/hooks/childProcessAttachService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@ import { inject, injectable } from 'inversify';
import { DebugConfiguration, DebugSession, WorkspaceFolder } from 'vscode';
import { IApplicationShell, IDebugService, IWorkspaceService } from '../../../common/application/types';
import { noop } from '../../../common/utils/misc';
import { SystemVariables } from '../../../common/variables/systemVariables';
import { captureTelemetry } from '../../../telemetry';
import { EventName } from '../../../telemetry/constants';
import { AttachRequestArguments, LaunchRequestArguments } from '../../types';
import { ChildProcessLaunchData, IChildProcessAttachService } from './types';
import { AttachRequestArguments } from '../../types';
import { IChildProcessAttachService } from './types';

/**
* This class is responsible for attaching the debugger to any
Expand All @@ -29,49 +28,16 @@ export class ChildProcessAttachService implements IChildProcessAttachService {
) {}

@captureTelemetry(EventName.DEBUGGER_ATTACH_TO_CHILD_PROCESS)
public async attach(
data: ChildProcessLaunchData | (AttachRequestArguments & DebugConfiguration),
parentSession: DebugSession
): Promise<void> {
let debugConfig: AttachRequestArguments & DebugConfiguration;
let processId: number;
if (this.isChildProcessLaunchData(data)) {
processId = data.processId;
debugConfig = this.getAttachConfiguration(data);
} else {
debugConfig = data;
processId = debugConfig.subProcessId!;
}
public async attach(data: AttachRequestArguments & DebugConfiguration, parentSession: DebugSession): Promise<void> {
const debugConfig: AttachRequestArguments & DebugConfiguration = data;
Comment on lines +31 to +32
Copy link
Copy Markdown

@ericsnowcurrently ericsnowcurrently May 27, 2020

Choose a reason for hiding this comment

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

Suggested change
public async attach(data: AttachRequestArguments & DebugConfiguration, parentSession: DebugSession): Promise<void> {
const debugConfig: AttachRequestArguments & DebugConfiguration = data;
public async attach(data: AttachRequestArguments & DebugConfiguration, parentSession: DebugSession): Promise<void> {
const debugConfig = data;

or

Suggested change
public async attach(data: AttachRequestArguments & DebugConfiguration, parentSession: DebugSession): Promise<void> {
const debugConfig: AttachRequestArguments & DebugConfiguration = data;
public async attach(debugConfig: AttachRequestArguments & DebugConfiguration, parentSession: DebugSession): Promise<void> {

const processId = debugConfig.subProcessId!;
const folder = this.getRelatedWorkspaceFolder(debugConfig);
const launched = await this.debugService.startDebugging(folder, debugConfig, parentSession);
if (!launched) {
this.appShell.showErrorMessage(`Failed to launch debugger for child process ${processId}`).then(noop, noop);
}
}
/**
Copy link
Copy Markdown

@ericsnowcurrently ericsnowcurrently May 27, 2020

Choose a reason for hiding this comment

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

I'm loving all this deleted code. 😄

* Since we're attaching we need to provide path mappings.
* If not provided, we cannot add breakpoints as we don't have mappings to the actual source.
* This is because attach automatically assumes remote debugging.
* Also remember, this code gets executed only when dynamically attaching to child processes.
* Resolves https://github.com/microsoft/vscode-python/issues/3568
*/
public fixPathMappings(config: LaunchRequestArguments & AttachRequestArguments & DebugConfiguration) {
if (!config.workspaceFolder) {
return;
}
if (Array.isArray(config.pathMappings) && config.pathMappings.length > 0) {
return;
}
// If user has provided a `cwd` in their `launch.json`, then we need to use
// the `cwd` as the localRoot.
// We cannot expect the debugger to assume remote root is the same as the cwd,
// As debugger doesn't necessarily know whether the process being attached to is
// a child process or not.
const systemVariables = new SystemVariables(undefined, config.workspaceFolder);
const localRoot =
config.cwd && config.cwd.length > 0 ? systemVariables.resolveAny(config.cwd) : config.workspaceFolder;
config.pathMappings = [{ remoteRoot: '.', localRoot }];
}

private getRelatedWorkspaceFolder(
config: AttachRequestArguments & DebugConfiguration
): WorkspaceFolder | undefined {
Expand All @@ -81,21 +47,4 @@ export class ChildProcessAttachService implements IChildProcessAttachService {
}
return this.workspaceService.workspaceFolders!.find((ws) => ws.uri.fsPath === workspaceFolder);
}
private getAttachConfiguration(data: ChildProcessLaunchData): AttachRequestArguments & DebugConfiguration {
const args = data.rootStartRequest.arguments;
// tslint:disable-next-line:no-any
const config = (JSON.parse(JSON.stringify(args)) as any) as AttachRequestArguments & DebugConfiguration;
// tslint:disable-next-line: no-any
this.fixPathMappings(config as any);
config.host = args.request === 'attach' ? args.host! : 'localhost';
config.port = data.port;
config.name = `Child Process ${data.processId}`;
config.request = 'attach';
return config;
}
private isChildProcessLaunchData(
data: ChildProcessLaunchData | (AttachRequestArguments & DebugConfiguration)
): data is ChildProcessLaunchData {
return data.rootStartRequest !== undefined;
}
}
1 change: 0 additions & 1 deletion src/client/debugger/extension/hooks/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

export enum DebuggerEvents {
// Event sent by PTVSD when a child process is launched and ready to be attached to for multi-proc debugging.
ChildProcessLaunched = 'ptvsd_subprocess',
PtvsdAttachToSubprocess = 'ptvsd_attach',
DebugpyAttachToSubprocess = 'debugpyAttach'
}
50 changes: 2 additions & 48 deletions src/client/debugger/extension/hooks/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,61 +4,15 @@
'use strict';

import { DebugConfiguration, DebugSession, DebugSessionCustomEvent } from 'vscode';
import { AttachRequestArguments, LaunchRequestArguments } from '../../types';
import { AttachRequestArguments } from '../../types';

export const IDebugSessionEventHandlers = Symbol('IDebugSessionEventHandlers');
export interface IDebugSessionEventHandlers {
handleCustomEvent?(e: DebugSessionCustomEvent): Promise<void>;
handleTerminateEvent?(e: DebugSession): Promise<void>;
}

export type ChildProcessLaunchData = {
/**
* The main process (that in turn starts child processes).
* @type {number}
*/
rootProcessId: number;
/**
* The immediate parent of the current process (identified by `processId`).
* This could be the same as `parentProcessId`, or something else.
* @type {number}
*/
parentProcessId: number;
/**
* The process id of the child process launched.
* @type {number}
*/
processId: number;
/**
* Port on which the child process is listening and waiting for the debugger to attach.
* @type {number}
*/
port: number;
/**
* The request object sent to the PTVSD by the main process.
* If main process was launched, then `arguments` would be the launch request arsg,
* else it would be the attach request args.
* @type {({
* // tslint:disable-next-line:no-banned-terms
* arguments: LaunchRequestArguments | AttachRequestArguments;
* command: 'attach' | 'request';
* seq: number;
* type: string;
* })}
*/
rootStartRequest: {
// tslint:disable-next-line:no-banned-terms
arguments: LaunchRequestArguments | AttachRequestArguments;
command: 'attach' | 'request';
seq: number;
type: string;
};
};

export const IChildProcessAttachService = Symbol('IChildProcessAttachService');
export interface IChildProcessAttachService {
attach(
data: ChildProcessLaunchData | (AttachRequestArguments & DebugConfiguration),
parentSession: DebugSession
): Promise<void>;
attach(data: AttachRequestArguments & DebugConfiguration, parentSession: DebugSession): Promise<void>;
}
33 changes: 8 additions & 25 deletions src/test/debugger/extension/hooks/childProcessAttachHandler.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { anything, capture, instance, mock, verify, when } from 'ts-mockito';
import { ChildProcessAttachEventHandler } from '../../../../client/debugger/extension/hooks/childProcessAttachHandler';
import { ChildProcessAttachService } from '../../../../client/debugger/extension/hooks/childProcessAttachService';
import { DebuggerEvents } from '../../../../client/debugger/extension/hooks/constants';
import { ChildProcessLaunchData } from '../../../../client/debugger/extension/hooks/types';
import { AttachRequestArguments } from '../../../../client/debugger/types';

suite('Debug - Child Process', () => {
test('Do not attach if the event is undefined', async () => {
Expand All @@ -27,14 +27,6 @@ suite('Debug - Child Process', () => {
await handler.handleCustomEvent({ event: 'abc', body, session });
verify(attachService.attach(body, session)).never();
});
test('Do not attach to child process if ptvsd_subprocess event is invalid', async () => {
const attachService = mock(ChildProcessAttachService);
const handler = new ChildProcessAttachEventHandler(instance(attachService));
const body: any = {};
const session: any = {};
await handler.handleCustomEvent({ event: DebuggerEvents.ChildProcessLaunched, body, session });
verify(attachService.attach(body, session)).never();
});
test('Do not attach to child process if ptvsd_attach event is invalid', async () => {
const attachService = mock(ChildProcessAttachService);
const handler = new ChildProcessAttachEventHandler(instance(attachService));
Expand All @@ -54,25 +46,16 @@ suite('Debug - Child Process', () => {
test('Exceptions are not bubbled up if exceptions are thrown', async () => {
const attachService = mock(ChildProcessAttachService);
const handler = new ChildProcessAttachEventHandler(instance(attachService));
const body: ChildProcessLaunchData = {
rootProcessId: 0,
parentProcessId: 0,
processId: 0,
port: 0,
rootStartRequest: {
arguments: {
type: 'python',
name: '',
request: 'attach'
},
command: 'attach',
seq: 0,
type: 'python'
}
const body: AttachRequestArguments = {
name: 'Attach',
type: 'python',
request: 'attach',
port: 1234,
subProcessId: 2
};
const session: any = {};
when(attachService.attach(body, session)).thenThrow(new Error('Kaboom'));
await handler.handleCustomEvent({ event: DebuggerEvents.ChildProcessLaunched, body, session: {} as any });
await handler.handleCustomEvent({ event: DebuggerEvents.DebugpyAttachToSubprocess, body, session: {} as any });
verify(attachService.attach(body, anything())).once();
const [, secondArg] = capture(attachService.attach).last();
expect(secondArg).to.deep.equal(session);
Expand Down
Loading