Skip to content
This repository has been archived by the owner on Nov 18, 2022. It is now read-only.

Child processes are not terminated #195

Open
schulzch opened this issue Nov 21, 2017 · 8 comments
Open

Child processes are not terminated #195

schulzch opened this issue Nov 21, 2017 · 8 comments

Comments

@schulzch
Copy link

Judging from the sources no child process seems to get killed if VS code exits - this is especially annoying if rls hangs on a laptop eating up battery unnoticed (e.g. #186 and friends). I guess one way to fix this would be by adding an exit handler like this:

process.on('exit', function() {
    child.kill();
});

The question is if you want to do this for all child processes or at all.

@nrc
Copy link
Member

nrc commented Nov 21, 2017

This is surprising. I would expect VSCode to always send an exit notification to the RLS for it to terminate. I wonder if VSCode docs offer any guidance for what to do here.

@schulzch
Copy link
Author

schulzch commented Nov 21, 2017

Taken from VS Code issue #948:

The right think to do here is for VSCode to call deactivate on extensions on shutdown and for extensions to clean up on shutdown.

Or more officially from VS Code docs:

Note: An extension must export an activate() function from its main module and it will be invoked only once by VS Code when any of the specified activation events is emitted. Also, an extension should export a deactivate() function from its main module to perform cleanup tasks on VS Code shutdown. Extension must return a Promise from deactivate() if the cleanup process is asynchronous. An extension may return undefined from deactivate() if the cleanup runs synchronously.

Sounds feasible, but I'm not sure if interrupting rustup is a great idea. Imagine someone installing the extension, then getting a update notification for VS Code, triggering a restart - et voila: corrupted rust installation?

@schulzch
Copy link
Author

I discovered VS Code issue #11895 and a repository to test issues related to limited deactivation time. Things get even worse with process hierarchies seemly having platform depended behavior (see detached).

I think I'll just write a wrapper that does the right thing and replace all instances of spawn() with it.

@schulzch
Copy link
Author

I've tried to implement it (commit), but apparently this is a lot harder to get right than I thought:

  • VS Code kills the extension host after a certain timeout (read: not enough time to kill more than one process on macOS with SIGTERM)
  • On macOS (and probably Linux) child processes are not killed with their parent (see node.js issue #2098)
  • And of course RLS spawns children

@schulzch
Copy link
Author

One solution might be to use tree-kill.

@kjeremy
Copy link
Contributor

kjeremy commented Aug 13, 2018

This still happens. I have two windows 10 machines that end up with multiple rls.exe processes even after vscode has exited.

@schulzch
Copy link
Author

I am also still bugged by this from time to time.

I think I'll give it another try the next couple of days using node-pty which also contains code to prevent console processes from leaking on Windows. Hopefully, having terminal semantics fixes issues with macOS as well.

@jens1o
Copy link

jens1o commented Aug 31, 2018

I can reproduce this by hitting Alt+F4 while rls reviews the dependencies, they continue to do their job.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants