Skip to content

fix: improve check for already exited processes on windows#464

Open
ceisele-r wants to merge 1 commit intobahmutov:masterfrom
ceisele-r:improve-already-exited-check
Open

fix: improve check for already exited processes on windows#464
ceisele-r wants to merge 1 commit intobahmutov:masterfrom
ceisele-r:improve-already-exited-check

Conversation

@ceisele-r
Copy link
Copy Markdown
Contributor

  • On Windows, already exited processes are not (always) reported as ESRCH, therefore match the error message to check if the process already exited

@MikeMcC399
Copy link
Copy Markdown
Collaborator

Thanks @ceisele-r for the contribution!

Is there any way to add this to the test suite? I'm just wondering how to judge whether the code is right or not.

@MikeMcC399 MikeMcC399 added the bug label May 7, 2026
@ceisele-r ceisele-r force-pushed the improve-already-exited-check branch 2 times, most recently from 84ea513 to 5d7d57c Compare May 7, 2026 10:16
* On Windows, already exited processes are not (always) reported as ESRCH, therefore match the error message to check if the process already exited
@ceisele-r ceisele-r force-pushed the improve-already-exited-check branch from 5d7d57c to ca7077d Compare May 7, 2026 10:22
@ceisele-r
Copy link
Copy Markdown
Contributor Author

Hi @MikeMcC399 , I've added a test that simulates this and tests the behavior.

@MikeMcC399
Copy link
Copy Markdown
Collaborator

Thanks for adding the tests! I'll approve this now and leave it open for comments for a few days. Based on past experience though I'm not really expecting any other reviews, but in the spirit of "open source" I'd like to give other users the chance to comment (including the owner of this repo by the way).

Copy link
Copy Markdown
Collaborator

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

Thank you very much for the enhancement!

@ceisele-r
Copy link
Copy Markdown
Contributor Author

Thank you @MikeMcC399 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants