Fix child processes not being killed properly

vpyatnitskiy commented 4 months ago

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

Simple steps to reproduce:

Launch VS Code.

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

Close VS Code.

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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

https://github.com/DaelonSuzuka/godot-vscode-plugin/commit/557fb2975ffbc3d69739b651df1d691ac0c7bb57 https://github.com/DaelonSuzuka/godot-vscode-plugin/commit/c8ccef5b2559b855696901ab6e950a0534394abb https://github.com/DaelonSuzuka/godot-vscode-plugin/commit/446b6f1df003d4542136af0f5e678816bb2a219f

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 commented 3 months ago

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:

https://github.com/godotengine/godot-vscode-plugin/blob/e89cb784da3b261ea3ca9955b6ea08b2dc4e61b9/src/debugger/godot4/server_controller.ts#L230

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 commented 3 months ago

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 commented 3 months ago

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

DaelonSuzuka commented 3 months ago

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

godotengine / godot-vscode-plugin

Fix child processes not being killed properly #613