do not sleep to kill a process that has already cleaned up and exited by itself
This PR fixes #671 by replacing the time.Sleep in killCmd (for linux and unix/mac) with a cancelable context that can be used to stop the kill timer and return to the caller as soon as the interrupted process has closed down.
While developing this PR I took the liberty of deleting the unit test for testing killing of "non existing" processes for reasons outlined in 52428bf7f43a18e0e96774071cccbf761d8bb1ca.
This PR has been tested on wsl2 linux and an Apple M2 MBP.
@xiantang What is the status/probability of accepting this PR?
could u rebase to master?
Because I'm doing https://github.com/air-verse/air/pull/809, I think there may be conflicts.
I need to merge #809 to master first, because once I remove the pty, Linux behavior will be broken, and it cannot clean child processes correctly.
I need to merge #809 to master first, because once I remove the pty, Linux behavior will be broken, and it cannot clean child processes correctly.
I do not understand. Why would you merge something that breaks Linux behaviour?
Oh, I see, you just changed the killcmd code, it should be okay. because for linux is still using pty to start a new process
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
| Files with missing lines | Coverage Δ | |
|---|---|---|
| runner/util_linux.go | 69.90% <100.00%> (+1.90%) |
:arrow_up: |
| runner/util_unix.go | 84.90% <100.00%> (+16.90%) |
:arrow_up: |
... and 1 file with indirect coverage changes
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Hello @istyf, could you help resolve the conflict?
Upstream merged into PR and conflict resolved. However this has now not been tested on my side.
Okay, please test this feature again as well, thanks.