Fix child processes not being killed properly

[vpyatnitskiy]

Currently, headless LSP processes continue running when VS Code exits on Linux.

Simple steps to reproduce:

1. Launch VS Code.

2. Check the headless processes ― for example, with ps ajx | grep headless.
   Here's the output on my machine:

   vpyatnitskiy@vpyatnitskiy-katana:~/tmp/godot-vscode-plugin$ ps ajx | grep headless
     26910   27154   27154   27154 ?             -1 Ss    1000   0:00 /bin/sh -c "/home/vpyatnitskiy/.local/share/Steam/steamapps/common/Godot Engine/godot.x11.opt.tools.64" --path "/home/vpyatnitskiy/godot-projects/Left or Right" --editor --headless --no-window --lsp-port 38661
     27154   27159   27154   27154 ?             -1 Rl    1000   0:01 /home/vpyatnitskiy/.local/share/Steam/steamapps/common/Godot Engine/godot.x11.opt.tools.64 --path /home/vpyatnitskiy/godot-projects/Left or Right --editor --headless --no-window --lsp-port 38661
      2292   27235   27234    2292 pts/1      27234 S+    1000   0:00 grep --color=auto headless
   

3. Close VS Code.

4. Check the processes again. Here's my output:

   vpyatnitskiy@vpyatnitskiy-katana:~/tmp/godot-vscode-plugin$ ps ajx | grep headless
         1   27159   27154   27154 ?             -1 Sl    1000   0:03 /home/vpyatnitskiy/.local/share/Steam/steamapps/common/Godot Engine/godot.x11.opt.tools.64 --path /home/vpyatnitskiy/godot-projects/Left or Right --editor --headless --no-window --lsp-port 38661
      2292   27290   27289    2292 pts/1      27289 R+    1000   0:00 grep --color=auto headless
   

As you can see, the shell process (/bin/sh) is gone, but the engine itself continues running. Each of those is half a gig of RAM ― relaunch a few times and it adds up.

What happens here is:

• shell: true causes child_process.spawn() to start a shell process which, in turn, starts Godot Engine. The returned PID is the shell's PID.

• Child processes are allowed to persist even when detached (see Node.js documentation on options.detached) ― so process.kill()ing the shell does not kill the engine.

Now, options.detached in this case makes Node start a new process group with the shell process as its leader; to terminate everything, the entire group needs to be killed. In the snippets above, you can see the group ID in the third column.

Process group IDs in Linux are equal to the PIDs of their leaders; to send a signal to a group, negative values for PID are used. See e.g. this answer on StackOverflow.

This PR makes it so that the kill signal is indeed dispatched to the group. It's a single character change, but I feel like it deserved a detailed explanation.

[DaelonSuzuka]

I had that minus sign at one point and I vaguely recall that it caused a problem on either Windows or MacOS.

I'll have to look into this to remember why I removed it.

[vpyatnitskiy]

Sure. If I'm reading this correctly, the else branch should not execute on Windows nor MacOS, so it's possible that it was masking an unrelated problem.

[LeoDog896]

This was a slight dig, but after finding the originating pull request on the change (#452)

DaelonSuzuka@557fb29
DaelonSuzuka@c8ccef5
DaelonSuzuka@446b6f1

There appeared to be an issue with using the group PID - perhaps it didn't work at all?

If I get to it, I'll also try investigating that in Linux.

[vpyatnitskiy]

I see. That seems to be debugger-related? Both the debugger and the headless LSP use the same utils/subspawn.ts routines to manage child processes, and the commits you linked seem to be surrounded by other work done on the debugger.

There's quite a lot going on in the linked PR, and I don't seem to find any discussion related to the linked commits.

However, there's in fact a similar ongoing issue with the debugger where the process under debug is not being terminated when the debugging session closes. It reproduces very reliably on my machine. It might be that the commits above were an attempt to fix this.

I wasn't able to locate the cause a week ago when I prepared this PR, but I took a second look just now, and this line seems to be the cause:

godot-vscode-plugin/src/debugger/godot4/server_controller.ts

Line 230 in e89cb78

const debugProcess = subProcess("debug", command, { shell: true });

This call makes the debugged process unkillable, because shell: true makes it run under a different PID, while the lack of detached: true also makes it run under its own, separate process group.

I tested on my machine, and can confirm that adding detached: true (combined with the minus sign from this PR) resolves the debugger problem.

I'm not in love with the idea of adding the fix to this PR, or other PR for that matter, because I'm not sure what other things I should test. What do you think?

[LeoDog896]

In that case, since this PR doesn't break the debugger's ability to properly kill its own process (as it is already broken), it seems fine to merge with an additional PR (tested by @DaelonSuzuka) that adds the detached header.

[vpyatnitskiy]

OK, I've sent a separate PR for this. Thank you!

[DaelonSuzuka]

@vpyatnitskiy Thanks for catching this problem and submitting a fix.