microsoft / vscode-debugadapter-node

Debug adapter protocol and implementation for VS Code.
Other
273 stars 79 forks source link

Debugee PID is not available to IDE #122

Closed DavidKarlas closed 7 years ago

DavidKarlas commented 7 years ago

When IDE requests adapter to start debugging some process it doesn't know PID of debugee. I think it should be reported back to IDE at some point.

gregg-miskelly commented 7 years ago

We currently have a protocol extension for sending back a process id in the one case we needed this.

How does this look?

    "ProcessCreateEvent": {
      "allOf": [
        { "$ref": "#/definitions/Event" },
        {
          "type": "object",
          "description": "Event message for 'processCreate' event type.",
          "properties": {
            "event": {
              "type": "string",
              "enum": [ "processCreate" ]
            },
            "body": {
              "type": "object",
              "properties": {
                "startMethod": {
                  "type": "string",
                  "description": "Describes how the debug engine started debugging this process -- 'launch', 'attach', or 'attachForSuspendedLaunch'."
                },
                "processId": {
                  "type": "integer",
                  "description": "The system process id of the debugged process. This property will be missing for non-system processes."
                }
              },
              "required": [ "startMethod" ]
            }
          },
          "required": [ "event", "body" ]
        }
      ]
    }
DavidKarlas commented 7 years ago

This looks like something I want yes... How do I use protocol extensions? Is this already part of vsdbg?

gregg-miskelly commented 7 years ago

Currently vsdbg will emit this event, but since it isn't part of the official protocol, and I didn't want to create a conflict in case something similar but not the same was added, we actually have code in vsdbg-ui to drop the event. If @weinand and @isidorn are happy with adding this to the official protocol I can just remove the code that drops it. If we think it doesn't make sense as an official event then I can tweak the code to emit it for VS For Mac. The protocol library that VS For Mac uses already supports it.

One note about processId: vsdbg doesn't understand when it is being used remotely. So it will emit this event even when remote debugging, in which case processId will be a process on some remote system.

andrewcrawley commented 7 years ago

@gregg-miskelly - If we're interested in getting this into the official protocol, I'd suggest changing the startMethod field to include a set of allowed values instead of just having it be a string with a comment listing the values we understand.

@DavidKarlas - On the hosting side, if you use the latest version of the NuGet package, it should have support for this event and any others we add.

gregg-miskelly commented 7 years ago

@andrewcrawley: Agreed on startMethod. I also think we need a note about processId.

weinand commented 7 years ago

just curious: what IDE functionality is based on the process ID?

In node-debug the process ID does not leave the debug adapter. So if the IDE wants to kill the debugee, it's done via a request and not by manipulating the process via a side channel through its ID (and potentially confusing the debug adapter).

andrewcrawley commented 7 years ago

In VS, the process ID is shown to the user in the "Processes" window. I can certainly imagine other reasons the host might want to know the process ID - for instance, maybe you'd want to show the CPU and memory usage of the debuggee somewhere. You'd need the PID to be able to use OS functions to get that info.

weinand commented 7 years ago

I'm planning to add the ProcessCreateEvent to the protocol. Here are some comments/questions/concerns:

gregg-miskelly commented 7 years ago

Renaming the event to 'ProcessEvent' sounds fine to me. I don't see any reason why 'startMethod' needs to be required. Description for 'attachForSuspendedLaunch': "A project launcher component has launched a new process in a suspended state and then asked the debugger to attach."

I will fix this up and send a PR.

weinand commented 7 years ago

@gregg-miskelly thanks for the PR.

I had to enhance my TS generator to support enumDescriptions...