Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use taskkill on Windows to kill all descendant processes on exit #3794

Closed
wants to merge 1 commit into from
Closed

Use taskkill on Windows to kill all descendant processes on exit #3794

wants to merge 1 commit into from

Conversation

Lyncredible
Copy link

This should fix issues like #3736 and #3763 where bash or zsh is
manually started inside cmd.exe. It also works for other long
running processes like SSH sessions inside cmd.exe or wsl.exe.

This should fix issues like #3736 and #3763 where bash or zsh is
manually started inside cmd.exe. It also works for other long
running processes like SSH sessions inside cmd.exe or wsl.exe.
Copy link

@mf mf left a comment

Choose a reason for hiding this comment

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

Looks good!

@Stanzilla
Copy link
Collaborator

Would this not cause a problem if you did this intentionally? Maybe some people want to start a long running process and close the terminal?

@Lyncredible
Copy link
Author

Lyncredible commented Sep 24, 2019

That is a good question, though I think this is the right behavior. child_process.kill() kills all descendant processes on Linux and Mac, but it does not work as advertised on Windows. This is the workaround suggested by the Node team, and will make the behavior consistent across all platforms.

In the *nix world, background jobs are shutdown when a shell is exited. If a user wants to keep a job running even after the terminal is closed, they should use something like the disown or nohup command to dissociate the job with the current terminal.

The Windows equivalent seems to be start, though that will create a new terminal window.

@ivanwonder
Copy link
Contributor

I think it's the problem of node-pty, see the pr #3781, it has been merged, you can update your code and try whether the problem has resolved or not. @Lyncredible

@Lyncredible
Copy link
Author

@ivanwonder That did the trick. I will close this PR. Thank you!

@Stanzilla If you look at the node-pty PR(microsoft/node-pty#262) that fixed the issue, they are also trying to kill the entire process tree 😃

@Stanzilla
Copy link
Collaborator

Ah, great we figured it out, thanks guys!

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

Successfully merging this pull request may close these issues.

4 participants