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

feat(win): process.kill / kill_with - add /t to kill tree (inc child … #1341

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

YOU54F
Copy link

@YOU54F YOU54F commented Aug 21, 2024

…processes)

See related thread pact-foundation/pact-plugins#73 (comment)

We use sysinfo to kill child processes (plugins invoked from our framework written in rust), across platforms.

plugins on windows may use a batch script to invoke the plugin (my case in reference was a java project). When using this project and process.kill, only the parent process is removed, and not any child processes invoked from the batch script

Taskkill has an option /t

/t : Specifies to terminate all child processes along with the parent process, commonly known as a tree kill.

https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-xp/bb491009(v=technet.10)?redirectedfrom=MSDN

Passing this flag, allows child processes to also be killed and doesn't appear to have any detrimental effect on existing behaviour (if there are no child processes)

@GuillaumeGomez
Copy link
Owner

This is exactly the wanted behaviour. However, we can extend the API to make it kill children as well. Adding a kill_children: bool argument to both functions should be enough. But then on non-windows platforms, it'll require to go through processes recursively and kill them all one by one.

@YOU54F
Copy link
Author

YOU54F commented Aug 21, 2024

This is exactly the wanted behaviour.

Thanks for confirming 👍🏾

However, we can extend the API to make it kill children as well.

🙏🏾

kill_children: bool argument to both functions should be enough.

That sounds like a sensible proposal

But then on non-windows platforms, it'll require to go through processes recursively and kill them all one by one.

Ahhh, I didn't actually realise that unix based systems doesn't kill child processes, the reason why I hadn't noticed in my testing, is the particular java plugin uses https://github.com/sbt/sbt-native-packager which creates native wrapper scripts. In the bash script wrapper, they use exec to call the java process invoked in the script, which gets the parent PID, when this is killed, the script releases from the exec, grabs the exit code from that process and exits with that code. They even have a note on the exec command in the wrapper script

execRunner () {
  # print the arguments one to a line, quoting any containing spaces
  [[ $verbose || $debug ]] && echo "# Executing command line:" && {
    for arg; do
      if printf "%s\n" "$arg" | grep -q ' '; then
        printf "\"%s\"\n" "$arg"
      else
        printf "%s\n" "$arg"
      fi
    done
    echo ""
  }

  # we use "exec" here for our pids to be accurate.
  exec "$@"
}

I'll look to make the changes tomorrow

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.

2 participants