Skip to content

core: gracefully kill process tree on exit#8947

Merged
paul-marechal merged 1 commit intomasterfrom
mp/exit-kill
Jun 29, 2021
Merged

core: gracefully kill process tree on exit#8947
paul-marechal merged 1 commit intomasterfrom
mp/exit-kill

Conversation

@paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Jan 14, 2021

What it does

See commit messages.

Fixes #8946

How to test

(Linux and maybe Mac)

  1. Run the Theia backend
  2. Connect with your browser to the backend in order to spawn the plugin host process
  3. Send SIGTERM to the backend process
  4. You can use htop to see that the plugin-host process is not running anymore.

On Windows I don't really know if this is an issue. But if anything, nothing should be broken on Windows.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal force-pushed the mp/exit-kill branch 2 times, most recently from 056b682 to c696e8f Compare January 14, 2021 01:56
@paul-marechal
Copy link
Member Author

paul-marechal commented Jan 14, 2021

I still need to figure out why the test is failing on GH Actions...

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Jan 14, 2021

I can confirm that this works in RHEL in an unpackaged electron app, and is able to kill previously recalcitrant processes (a trace server launched from this extension). However, in a packaged Electron app, the process configuration looks the same - the backend main process is the group leader for its descendants - but the backend main process and the trace server survive the application exit.

colin-grant-work
colin-grant-work reviewed Jan 14, 2021
View reviewed changes
colin-grant-work
colin-grant-work reviewed Jan 14, 2021
View reviewed changes
@paul-marechal paul-marechal force-pushed the mp/exit-kill branch 5 times, most recently from 94b84db to bbe4a5b Compare January 15, 2021 05:38
colin-grant-work
colin-grant-work reviewed Jan 15, 2021
View reviewed changes
@paul-marechal paul-marechal force-pushed the mp/exit-kill branch 2 times, most recently from 52ebef0 to 8c1e86e Compare January 15, 2021 17:44
colin-grant-work
colin-grant-work reviewed Jan 15, 2021
View reviewed changes
@paul-marechal paul-marechal force-pushed the mp/exit-kill branch 2 times, most recently from 8ea7df7 to b3593a0 Compare January 15, 2021 19:49
@kittaakos
Copy link
Contributor

kittaakos commented Jan 18, 2021

How does VS Code handle this? Do they also have such sophisticated logic for terminating the process tree? Can we use the process termination logic as-is from the language client? I'd prefer that tested code over some in-house solution.

kittaakos
kittaakos reviewed Jan 18, 2021
View reviewed changes
@colin-grant-work
Copy link
Contributor

colin-grant-work commented Jan 18, 2021 •
edited
Loading

How does VS Code handle this? Do they also have such sophisticated logic for terminating the process tree? Can we use the process termination logic as-is from the language client? I'd prefer that tested code over some in-house solution.

I very often use two libs and the following snippet to terminate a process (tree). It works for me reliably.

Both the VSCode shell script that's called in the code you linked and the ps-tree/tree-kill code depend on consistent parents, so it would still likely be desirable to supplement them with the group-ID-oriented code, but I think for finding the children recursively, psTree is likely a good idea.

Forgot about the need for everything to be sync.

@paul-marechal paul-marechal force-pushed the mp/exit-kill branch 4 times, most recently from 8b83d50 to c4e9d1e Compare June 22, 2021 23:03
colin-grant-work
colin-grant-work approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this code in a Linux environment and confirmed that it successfully kills processes that are left hanging by version 1.14.0.

@paul-marechal
core: gracefully kill process tree on exit
133b7d8 
When killing a given process, its children are supposed to keep running
unaffected. Since we never kill our subprocesses ourselves, we often end
up leaking orphaned processes. The most notorious is the plugin-host
process.

When running the browser version, yarn somehow ends up cleaning up for
us. But when running Electron, simply killing the backend process is not
enough, we need to find a way to kill everything.

This commit addresses this issue, with some platform specifities:

- On Windows we'll run `taskkill /f /t /pid xxx` in order to destroy
  the whole tree from the backend pid.
- On UNIX we'll prefer starting the backend process as group leader,
  this way we can kill the whole group at once. But we'll still also
  follow parent/child relationships in case some processes are spawned
  in a different group.

Signed-off-by: Paul <paul.marechal@ericsson.com>
@paul-marechal paul-marechal merged commit 0ebdec5 into master Jun 29, 2021
@paul-marechal paul-marechal deleted the mp/exit-kill branch June 29, 2021 05:53
@github-actions github-actions bot added this to the 1.15.0 milestone Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colin-grant-work colin-grant-work colin-grant-work approved these changes

@vince-fugnitto vince-fugnitto Awaiting requested review from vince-fugnitto

+1 more reviewer

@kittaakos kittaakos kittaakos left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Orphaned processes after killing the backend

3 participants

@paul-marechal @colin-grant-work @kittaakos