Use SIGTERM instead of SIGKILL when ending processes, allow attachSimplePort=0

[alexkli]

Describe the bug

(Note sure if bug or feature request is most appropriate, this might be a somewhat special case!)

wskdebug is a debugging tool for Apache Openwhisk (a serverless platform), which starts a container with a nodejs process inside that will have the actual debug port to which the VS Code debugger should connect. I am using this in VS Code as runtimeExecutable and have been intercepting the --inspect-brk=NNNN argument that was set by the previous debugger to use this port on the container that gets started. VS Code then did connect to the container, which was great.

However, it seems the new debugger uses a different mechanism. Apparently it sets NODE_OPTIONS with a --require that loads a JS file into the node process that apparently sets up the debugging from "inside" the process.

The problem is that now it debugs the wskdebug "intermediary" and not the target container.

Is there a way in the new debugger I can disable that and just get hold off the port number (which VS code randomly chooses, i.e. in case there are multiple debug sessions going on)?

See also apache/openwhisk-wskdebug#74

On a related note, the new debugger seems to kill the process at the end immediately (SIGKILL style). The previous debugger sent a SIGTERM so that the process had a chance to cleanup and properly exit. In wskdebug case this is used to stop the docker container amongst other things. With the new debugger this is no longer possible and thus the container with the open debug port is left behind, so that the next run usually fails with "debug port already in use". This is also an artifact of the above issue since always the same default debug port (9229) is picked now since --inspect-brk=NNNN with a unique port NNNN is missing.

To Reproduce
Steps to reproduce the behavior:

1. Run launch config with wskdebug as described here
2. Notice debugger connects to wskdebug itself, but not the container it starts
3. Cannot hit any breakpoints since it's debugging the wrong process

Log File

vscode-debugadapter-9.json.gz

VS Code Version: 1.47.1

[connor4312]

Thanks for the details. I think your scenario and fix is covered in #586 (comment). If attachSimplePort is set, we will also not filter out --inspect-brk as of today's nightly (5PM PST). However I'll leave this open to use SIGTERM instead of KILL.

[alexkli]

@connor4312 Thanks so much for your quick reply!

I tried the nightly + attachSimplePort but it did not fix the issue. VS code still invokes the program without the --inspect-brk=NNNN argument.

How exactly do I set it in the launch.json? I have this but the editor underlines it with a warning Incorrect type. Expected "number":

      "port": "attachSimplePort"

I am using version v2020.7.1517 of @id:ms-vscode.js-debug-nightly.

[connor4312]

The nightly where --inspect-brk will not be filtered is today's nightly, which will be available in about two hours from now 🙂

The change you want in your launch.json is

{
  "type": "pwa-node", // if only "node", VS Code will complain
  "request": "launch",
  "name": "My launch",
+ "attachSimplePort": 9229,

[alexkli]

Ah, sorry, I missed the "today's nightly" part!

Regarding:

"attachSimplePort": 9229

Unfortunately that forces me to pick a port and hardcode it into each launch config, which puts some burden on the user.

Especially if there are multiple launch configs (which is likely in our use case, one might debug multiple serverless functions in parallel), which all would need a different port number. And also if another debug session is going on or simply a leftover nodejs/container is still running in the background, the auto selection nicely handled those issues.

[connor4312]

I think we could do something like -- if attachSimplePort is set to 0, then find some open port and assign it, and add --inspect-brk=thatPort to the runtime args.

[connor4312]

Added in the linked commit, which will be in the nightly. Just assign attachSimplePort to 0, and js-debug will automatically pick a port and add --inspect-brk as it did previously. Note that with this you must now have the type be pwa-node, otherwise it won't go through js-debug's config resolver where this logic happens.

[alexkli]

Can confirm that attachSimplePort works in the nightly 🥳.

For reference, here is the launch.json snippet I used:

      "type": "pwa-node",
      "request": "launch",
      "name": "wskdebug",
      "attachSimplePort": 0,

[alexkli]

@connor4312 Hi there, I was wondering if the changes for pwa-node and attachSimplePort are available in the July 2020 VS Code release aka the 1.48.x version?

[connor4312]

Yep, they are

[jkoput]

Is the change back to SIGTERM/SIGINT instead of SIGKILL on the roadmap? This is impacting a current development; would we be better off looking at a different IDE?

[connor4312]

I do not have an ETA for this. However PRs are welcome.

[zgz682000]

Why deprecate sending SIGTERM/SIGINT, without them I don't know how to gracefully shutdown my node server when debuging with vscode. Have anyone ideas?

[connor4312]

You can now set "killBehavior": "polite" to use SIGTERM on posix. On windows this has the effect of omitted the /F force flag from taskkill.exe.

[roblourens]

Is this like the thing that's supposed to happen with the terminateRequest? From what I remember, the DA can opt in to first get a terminateRequest whenthe user clicks the stop button, then if the program doesn't terminate, the user can click the stop button a second time and the DA can try again with the stronger signal. microsoft/vscode#57018 (comment)

[roblourens]

The current behavior with killBehavior=polite leaks the process (if it intercepts the signal), that seems not great

[connor4312]

The current behavior with killBehavior=polite leaks the process (if it intercepts the signal), that seems not great

I think that's up to the user to deal with in that case. If they opt in so that js-debug terminates their process gracefully and it fails to shut down as a result of SIGTERM, that's an issue that needs to be fixed in their program. We could introduce a heuristic that forcefully kills the process if it remains alive after some timeout, and I think that would be good if SIGTERM was the default behavior, but because it's opt-in I'm not sure that level of extra configuration for the timeout is needed.

[roblourens]

That's fine. How about #630 (comment) though?

[connor4312]

It looks like can happen, but js-debug does not have supportsTermianteRequest. I'll open a new issue for that since this thread has several different branches, but I think that's better than specifying the kill behavior.