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

fix: update windows for new changes #38

Merged
merged 6 commits into from
May 29, 2024

Conversation

jacobtread
Copy link
Contributor

@jacobtread jacobtread commented May 21, 2024

Looks like a fair bit of unix-specific code has gotten tangled up in the main files

Description of the changes

Updated windows support for the latest changes that came in on the unix side of things

  • Have separated the unix specific process logic into the unix.rs module
  • Created a wrapper for the Signal type since its not available on non unix platforms, non unix platforms just handle it as a uppercase string (No special handling at this stage, but could be implemented at a later stage if we make our own signal enum that maps to the unix enum)
  • Added a new WindowsProcess type for windows processes,
  • Wired up the code to collect and kill in separate places
  • Added process name lookup for windows processes (To match the other impls) if a process has a bad name or fails to obtain its name None is used instead
  • Docker container killing is working
  • Made the type for a killable into an enum as its possible values are constant?
  • Renamed the killport_tests.rs to killport_unix_tests.rs and gated it to unix only since its using unix specific features, Windows tests will need to be added
  • Added a build and release target for windows to the github workflows
  • Added windows tests
  • Updated integration tests to handle windows process names

Related issue(s)
If this PR is related to an existing issue, please link to it using the Fixes #issue_number or Closes #issue_number syntax.

Type of change
Please select one or multiple of the following options:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code cleanup (refactoring or improving code quality)
  • Documentation update (adding or updating documentation, updating README)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Additional information
Add any other information or screenshots about the pull request here.

Looks like a fair bit of unix-specific code has gotten tangled up in the main files

Have separated the unix specific process logic into the unix.rs module

Created a wrapper for the Signal type since its not available on non unix platforms, non unix platforms just handle it as a uppercase string (No special handling at this stage, but could be implemented at a later stage if we make our own signal enum that maps to the unix enum)

Added a new WindowsProcess type for windows processes,

Wired up the code to collect and kill in separate places

Added process name lookup for windows processes (To match the other impls) if a process has a bad name or fails to obtain its name None is used instead

Docker container killing is working, however it doesn't work properly if you don't explicitly specify the container mode (I think its trying to kill the wrong one causing it to fail?)

Made the type for a killable into an enum as its possible values are constant?

Renamed the killport_tests.rs to killport_unix_tests.rs and gated it to unix only since its using unix specific features, Windows tests will need to be added

Added a build and release target for windows to the github workflows
@jacobtread
Copy link
Contributor Author

I have noticed a flaw in the previous algorithm I used for resolving the parent processes, it would only manage to catch some of the parents since it only did a single pass over the collection of processes (So it would only pick up the one parent or maybe more if they were in the entries after)

I implemented a proper version that catches all the processes and I've realized very quickly that we don't want to be killing all the parent processes. (Long story short it ends up killing explorer.exe and winlogon.exe)

So I've limited it to only resolving 1 parent using a constant named MAX_PARENT_DEPTH for choosing how deep to resolve the parents. We might want to gate killing the parent processes behind a optional argument, that provides the max depth?

@jacobtread jacobtread marked this pull request as ready for review May 22, 2024 09:39
@jkfran
Copy link
Owner

jkfran commented May 22, 2024

Thank you so much for this.

So I've limited it to only resolving 1 parent using a constant named MAX_PARENT_DEPTH for choosing how deep to resolve the parents. We might want to gate killing the parent processes behind a optional argument, that provides the max depth?

Killing the parents is missing on Linux and Macos as well. This could be an optional argument we could add into it in the future. Feel free to not kill parents at the moment. One question what happens if the process doesn't have any parents and MAX_PARENT_DEPTH=1 will it kill explorer.exe or winlogon.exe?

@jacobtread
Copy link
Contributor Author

Yeah, you're right. If theres no other parents, it'd still kill Explorer, I think we had this behavior because of a nginx process that would auto restart itself. I will disable this tonight for now. I will leave the code around for resolving parents just in case we want it later

Depth to zero for now to prevent killing parent processes
@jacobtread
Copy link
Contributor Author

I have set the MAX_PARENT_DEPTH const to zero for now so it won't kill any parents :)

Should be all good to go!

@jkfran jkfran merged commit 0412124 into jkfran:main May 29, 2024
12 checks passed
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.

Make v1 available on Window
2 participants