Skip to content

Attach to local process - launch.json update #8831

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

Merged
merged 6 commits into from
Nov 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions news/1 Enhancements/8384.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add "processId" key in launch.json to enable attach-to-local-pid scenarios when using the new debug adapter.
7 changes: 4 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1287,9 +1287,6 @@
}
},
"attach": {
"required": [
"port"
],
"properties": {
"port": {
"type": "number",
Expand Down Expand Up @@ -1364,6 +1361,10 @@
"type": "boolean",
"description": "Show return value of functions when stepping.",
"default": true
},
"processId": {
"type": "number",
"description": "ID of the local process to attach to."
}
}
}
Expand Down
19 changes: 15 additions & 4 deletions src/client/debugger/extension/adapter/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,29 @@ export class DebugAdapterDescriptorFactory implements IDebugAdapterDescriptorFac
const configuration = session.configuration as (LaunchRequestArguments | AttachRequestArguments);

if (this.experimentsManager.inExperiment(DebugAdapterNewPtvsd.experiment)) {
if (configuration.request === 'attach') {
const port = configuration.port ? configuration.port : 0;
const isAttach = configuration.request === 'attach';
const port = configuration.port ?? 0;
// When processId is provided we may have to inject the debugger into the process.
// This is done by the debug adapter, so we need to start it. The adapter will handle injecting the debugger when it receives the attach request.
const processId = configuration.processId ?? 0;
Copy link

@DonJayamanne DonJayamanne Nov 28, 2019

Choose a reason for hiding this comment

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

Curious, if we have a check for 0 , then why not < 0
I think we can prevent user from entering 0 or -2 by using the json schema.
This way the code can validate whether is a number or not (deal with nullables, instead of 0 or not)

Copy link
Author

Choose a reason for hiding this comment

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

In the json schema port and processId are numbers, so there is already some validation going on. @karthiknadig's original logic was to set port's value to 0 if undefined since it isn't an accepted value anyway, I just applied it toprocessId as well.


if (isAttach && processId === 0) {
if (port === 0) {
throw new Error('Port must be specified for request type attach');
throw new Error('Port or processId must be specified for request type attach');
} else {
return new DebugAdapterServer(port, configuration.host);
}
return new DebugAdapterServer(port, configuration.host);
} else {
const pythonPath = await this.getPythonPath(configuration, session.workspaceFolder);
// If logToFile is set in the debug config then pass --log-dir <path-to-extension-dir> when launching the debug adapter.
const logArgs = configuration.logToFile ? ['--log-dir', EXTENSION_ROOT_DIR] : [];
const ptvsdPathToUse = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'lib', 'python', 'new_ptvsd');

if (pythonPath.length !== 0) {
if (processId) {
sendTelemetryEvent(EventName.DEBUGGER_ATTACH_TO_LOCAL_PROCESS);
}

if (await this.useNewPtvsd(pythonPath)) {
sendTelemetryEvent(EventName.DEBUG_ADAPTER_USING_WHEELS_PATH, undefined, { usingWheels: true });
return new DebugAdapterExecutable(pythonPath, [path.join(ptvsdPathToUse, 'wheels', 'ptvsd', 'adapter'), ...logArgs]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ export class ChildProcessAttachService implements IChildProcessAttachService {
public async attach(data: ChildProcessLaunchData | (AttachRequestArguments & DebugConfiguration), parentSession: DebugSession): Promise<void> {
let debugConfig: AttachRequestArguments & DebugConfiguration;
let processId: number;
if (data.rootStartRequest) {
if (this.isChildProcessLaunchData(data)) {
processId = data.processId;
debugConfig = this.getAttachConfiguration(data as ChildProcessLaunchData);
debugConfig = this.getAttachConfiguration(data);
} else {
debugConfig = data as (AttachRequestArguments & DebugConfiguration);
debugConfig = data;
processId = debugConfig.subProcessId!;
}
const folder = this.getRelatedWorkspaceFolder(debugConfig);
Expand Down Expand Up @@ -87,4 +87,7 @@ export class ChildProcessAttachService implements IChildProcessAttachService {
config.request = 'attach';
return config;
}
private isChildProcessLaunchData(data: ChildProcessLaunchData | (AttachRequestArguments & DebugConfiguration)): data is ChildProcessLaunchData {
return data.rootStartRequest !== undefined;
}
}
1 change: 1 addition & 0 deletions src/client/debugger/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface IKnownAttachDebugArguments extends ICommonDebugArguments {
// Internal files used to attach to subprocess using python debug adapter
subProcessId?: number;

processId?: number;
}

export interface IKnownLaunchRequestArguments extends ICommonDebugArguments {
Expand Down
1 change: 1 addition & 0 deletions src/client/telemetry/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export enum EventName {
DEBUG_SESSION_USER_CODE_RUNNING = 'DEBUG_SESSION.USER_CODE_RUNNING',
DEBUGGER = 'DEBUGGER',
DEBUGGER_ATTACH_TO_CHILD_PROCESS = 'DEBUGGER.ATTACH_TO_CHILD_PROCESS',
DEBUGGER_ATTACH_TO_LOCAL_PROCESS = 'DEBUGGER.ATTACH_TO_LOCAL_PROCESS',
DEBUGGER_CONFIGURATION_PROMPTS = 'DEBUGGER.CONFIGURATION.PROMPTS',
DEBUGGER_CONFIGURATION_PROMPTS_IN_LAUNCH_JSON = 'DEBUGGER.CONFIGURATION.PROMPTS.IN.LAUNCH.JSON',
UNITTEST_STOP = 'UNITTEST.STOP',
Expand Down
8 changes: 6 additions & 2 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ export interface IEventNamePropertyMapping {
*/
pyspark: boolean;
/**
* Whether using `gevent` when deugging.
* Whether using `gevent` when debugging.
*
* @type {boolean}
*/
Expand All @@ -488,12 +488,16 @@ export interface IEventNamePropertyMapping {
* Telemetry event sent when attaching to child process
*/
[EventName.DEBUGGER_ATTACH_TO_CHILD_PROCESS]: never | undefined;
/**
* Telemetry event sent when attaching to a local process.
*/
[EventName.DEBUGGER_ATTACH_TO_LOCAL_PROCESS]: never | undefined;
/**
* Telemetry sent after building configuration for debugger
*/
[EventName.DEBUGGER_CONFIGURATION_PROMPTS]: {
/**
* The type of debug configuration to build configuration fore
* The type of debug configuration to build configuration for
*
* @type {DebugConfigurationType}
*/
Expand Down
47 changes: 40 additions & 7 deletions src/test/debugger/extension/adapter/factory.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ suite('Debugging - Adapter Factory', () => {
assert.deepEqual(descriptor, nodeExecutable);
});

test('Return Debug Adapter server if attach configuration and we are in experiment', async () => {
test('Return Debug Adapter server if in DA experiment, configuration is attach and port is specified', async () => {
const session = createSession({ request: 'attach', port: 5678, host: 'localhost' });
const debugServer = new DebugAdapterServer(session.configuration.port, session.configuration.host);

Expand All @@ -194,22 +194,22 @@ suite('Debugging - Adapter Factory', () => {
assert.deepEqual(descriptor, debugServer);
});

test('Throw error if configuration is attach and in experiment and port is 0', async () => {
test('Throw error if in DA experiment, configuration is attach, port is 0 and process ID is not specified', async () => {
const session = createSession({ request: 'attach', port: 0, host: 'localhost' });

when(spiedInstance.inExperiment(DebugAdapterNewPtvsd.experiment)).thenReturn(true);
const promise = factory.createDebugAdapterDescriptor(session, nodeExecutable);

await expect(promise).to.eventually.be.rejectedWith('Port must be specified for request type attach');
await expect(promise).to.eventually.be.rejectedWith('Port or processId must be specified for request type attach');
});

test('Throw error if configuration is attach and in experiment and port is not specified', async () => {
const session = createSession({ request: 'attach', port: undefined });
test('Throw error if in DA experiment, configuration is attach and port and process ID are not specified', async () => {
const session = createSession({ request: 'attach', port: undefined, processId: undefined });

when(spiedInstance.inExperiment(DebugAdapterNewPtvsd.experiment)).thenReturn(true);
const promise = factory.createDebugAdapterDescriptor(session, nodeExecutable);

await expect(promise).to.eventually.be.rejectedWith('Port must be specified for request type attach');
await expect(promise).to.eventually.be.rejectedWith('Port or processId must be specified for request type attach');
});

test('Return old node debugger when not in the experiment', async () => {
Expand All @@ -219,6 +219,19 @@ suite('Debugging - Adapter Factory', () => {
assert.deepEqual(descriptor, nodeExecutable);
});

test('Return Python debug adapter without wheels executable if configuration is attach, process ID is specified and active interpreter is not Python 3.7', async () => {
const debugExecutable = new DebugAdapterExecutable(python36Path, [ptvsdAdapterPathWithoutWheels]);
const session = createSession({ request: 'attach', processId: 1234 });

when(spiedInstance.inExperiment(DebugAdapterNewPtvsd.experiment)).thenReturn(true);
when(interpreterService.getInterpreters(anything())).thenResolve([interpreterPython36Details]);
when(interpreterService.getInterpreterDetails(python36Path)).thenResolve(interpreterPython36Details);

const descriptor = await factory.createDebugAdapterDescriptor(session, nodeExecutable);

assert.deepEqual(descriptor, debugExecutable);
});

test('Return Python debug adapter without wheels executable when the active interpreter is not Python 3.7', async () => {
const debugExecutable = new DebugAdapterExecutable(python36Path, [ptvsdAdapterPathWithoutWheels]);
const session = createSession({});
Expand All @@ -232,6 +245,17 @@ suite('Debugging - Adapter Factory', () => {
assert.deepEqual(descriptor, debugExecutable);
});

test('Return Python debug adapter with wheels executable if configuration is attach, process ID is specified and active interpreter is Python 3.7', async () => {
const debugExecutable = new DebugAdapterExecutable(pythonPath, [ptvsdAdapterPathWithWheels]);
const session = createSession({ request: 'attach', processId: 1234 });

when(spiedInstance.inExperiment(DebugAdapterNewPtvsd.experiment)).thenReturn(true);

const descriptor = await factory.createDebugAdapterDescriptor(session, nodeExecutable);

assert.deepEqual(descriptor, debugExecutable);
});

test('Return Python debug adapter with wheels executable when in the experiment and with the active interpreter being Python 3.7', async () => {
const debugExecutable = new DebugAdapterExecutable(pythonPath, [ptvsdAdapterPathWithWheels]);
const session = createSession({});
Expand Down Expand Up @@ -332,7 +356,7 @@ suite('Debugging - Adapter Factory', () => {
assert.deepEqual(Reporter.properties, [{ expName: DebugAdapterNewPtvsd.experiment }, { usingWheels: 'true' }]);
});

test('Send experiment group telemetry if inside the wheels experiment, with active interpreter not Python 3.6', async () => {
test('Send experiment group telemetry if inside the wheels experiment, with active interpreter not Python 3.7', async () => {
const session = createSession({});
when(spiedInstance.userExperiments).thenReturn([{ name: DebugAdapterNewPtvsd.experiment, salt: DebugAdapterNewPtvsd.experiment, min: 0, max: 0 }]);
when(interpreterService.getInterpreters(anything())).thenResolve([interpreterPython36Details]);
Expand All @@ -344,6 +368,15 @@ suite('Debugging - Adapter Factory', () => {
assert.deepEqual(Reporter.properties, [{ expName: DebugAdapterNewPtvsd.experiment }, { usingWheels: 'false' }]);
});

test('Send attach to local process telemetry if inside the DA experiment and attaching to a local process', async () => {
const session = createSession({ request: 'attach', processId: 1234 });
when(spiedInstance.userExperiments).thenReturn([{ name: DebugAdapterNewPtvsd.experiment, salt: DebugAdapterNewPtvsd.experiment, min: 0, max: 0 }]);

await factory.createDebugAdapterDescriptor(session, nodeExecutable);

assert.ok(Reporter.eventNames.includes(EventName.DEBUGGER_ATTACH_TO_LOCAL_PROCESS));
});

test('Send control group telemetry if inside the DA experiment control group', async () => {
const session = createSession({});
when(spiedInstance.userExperiments).thenReturn([{ name: DebugAdapterNewPtvsd.control, salt: DebugAdapterNewPtvsd.control, min: 0, max: 0 }]);
Expand Down