Dynamic node2 debug protocol detection #150
Conversation
|
@weinand please take a look |
|
@roblourens thanks Rob, will do! |
src/node/extension/extension.ts
Outdated
| @@ -388,15 +386,31 @@ function startSession(config: any): StartSessionResult { | |||
| } | |||
| } | |||
|
|
|||
| let determineDebugTypeP: Promise<string|null>; | |||
| if (config.request === 'attach' && typeof config.processId === 'string' && config.processId.trim() === '${command:PickProcess}') { | |||
There was a problem hiding this comment.
The check for '${command:PickProcess}' shows the problematic aspect of the approach: instead of letting VS Code handle the variable substitution generically, the node-debug extension now does the substitution explicitly, before VS Code gets a chance for doing it. But I don't think that this will break anybody... so I'm not asking that we change this approach.
May be we should consider to check for the alternative command ${command:extension.pickNodeProcess} as well.
src/node/extension/extension.ts
Outdated
| return config.port === INSPECTOR_PORT_DEFAULT ? 'inspector' : | ||
| config.port === LEGACY_PORT_DEFAULT ? 'legacy' : | ||
| config.protocol ? config.protocol : | ||
| detectProtocolForPid(pidNum); |
There was a problem hiding this comment.
I suggest to use some 'if' statements here...
src/node/extension/extension.ts
Outdated
| return debugType; | ||
| }); | ||
| } else { | ||
| determineDebugTypeP = determineDebugType(config); |
There was a problem hiding this comment.
Why is the behaviour of determineDebugType so different from pickProcessAndDetermineDebugType?
I tried to use "processId": "12345" instead of "processId": "${command:PickProcess}" in my launch config (where 12345 is the ID of a node 8 process that I could have picked via the picker as well) but this fails because determineDebugType falls back to "legacy" (and I don't see a need for this). I would expect that picking a process and specifying its ID behave identically.
There was a problem hiding this comment.
I'm refactoring this a little to fix that case.
src/node/extension/extension.ts
Outdated
| config.port = debugType === 'node2' ? INSPECTOR_PORT_DEFAULT : LEGACY_PORT_DEFAULT; | ||
| } | ||
|
|
||
| return debugType; |
There was a problem hiding this comment.
When running two node 8 processes, sending SIGUSR1 to the second results in the (expected) message: "Starting inspector on 127.0.0.1:9229 failed: address already in use". As a consequence "lsof/netstat" cannot determine the port and the "debugType" above will be null. Currently there is no error message shown in VS Code (and the node 8 message is probably not always visible). So we should consider to improve the error reporting in the "startSession" command.
There was a problem hiding this comment.
Error reporting needs work, but I don't know how to handle that particular case, because the process.kill call doesn't fail. I think the only sign of it failing is the stderr message which we can't read in this case.
|
Thanks! |
Fixes microsoft/vscode#25036. Follows the process proposed in that issue, using
lsofon linux/mac andnetstaton windows.