air icon indicating copy to clipboard operation
air copied to clipboard

do not sleep to kill a process that has already cleaned up and exited by itself

Open istyf opened this issue 1 year ago • 10 comments

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.

istyf avatar Nov 01 '24 23:11 istyf

@xiantang What is the status/probability of accepting this PR?

istyf avatar Nov 10 '25 15:11 istyf

could u rebase to master?

xiantang avatar Nov 10 '25 15:11 xiantang

Because I'm doing https://github.com/air-verse/air/pull/809, I think there may be conflicts.

xiantang avatar Nov 10 '25 15:11 xiantang

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.

xiantang avatar Nov 10 '25 15:11 xiantang

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?

istyf avatar Nov 10 '25 15:11 istyf

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

xiantang avatar Nov 10 '25 16:11 xiantang

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.

codecov[bot] avatar Nov 10 '25 19:11 codecov[bot]

Hello @istyf, could you help resolve the conflict?

xiantang avatar Nov 12 '25 06:11 xiantang

Upstream merged into PR and conflict resolved. However this has now not been tested on my side.

istyf avatar Nov 12 '25 14:11 istyf

Okay, please test this feature again as well, thanks.

xiantang avatar Nov 12 '25 14:11 xiantang