-
Notifications
You must be signed in to change notification settings - Fork 191
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
Use Go's cross-platform process killing instead of the UNIX-only syscall.Kill #161
Conversation
logrus.Warn("failed finding instance, assuming instance has externally terminated", err) | ||
} | ||
|
||
if err := process.Signal(syscall.SIGKILL); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll get a panic here if process == nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out. I rechecked it. In Golang, the process
will never be nil
if err == nil
, you can check the three implementations for windows, unix and plan9 here: https://github.com/golang/go/search?q=findProcess%28pid+int%29&unscoped_q=findProcess%28pid+int%29
And Golang also officially assume it in its test:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but there's no early return here; you check if err != nil
, log a warning and then continue (in which case process == nil
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed it in the new commit. Please review.
logrus.Warn("failed finding instance, assuming instance has externally terminated", err) | ||
} | ||
|
||
if err := process.Signal(syscall.SIGKILL); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
192b555
to
990c28b
Compare
if err := syscall.Kill(pid, syscall.SIGKILL); err != nil { | ||
process, err := os.FindProcess(pid) | ||
if err != nil { | ||
return errors.New("failed to find process", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't necessarily want to return here; we just want to throw a warning and move on (for example if the process was stopped externally). we still want to reach the final line return p.state.RemoveInstance(instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the PR. Please review.
looks good! thanks for another contribution. |
The system call
syscall.Kill(pid, syscall.SIGKILL)
is not available on Windows. So I replace it with a more cross-platform way which supports all OSes. Now Unik can be compiled on Windows.