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
Supervisor on windows can "supervise" process for state change #1577
Conversation
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
5d82c5f
to
3b9652a
Compare
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.
This is very well thought out and I can completely tell that you looked at the standard library for guidance which is fucking great. Your understanding of both Rust and C is coming along super well. Thank you so much for tackling this one!
I've got a few comments inline. Could you also run this through the code formatter and re-push things up? Some of the indentations are off 🤓
@@ -47,3 +50,147 @@ fn become_child_command(command: PathBuf, args: Vec<OsString>) -> Result<()> { | |||
// Let's honor the exit codes from the child process we finished running | |||
process::exit(status.code().unwrap()) | |||
} | |||
|
|||
fn handle_from_pid(pid: u32) -> Option<winapi::HANDLE> { |
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.
Great use of Option here - null ptr definitely == None
|
||
pub struct Child { | ||
handle: Option<winapi::HANDLE>, | ||
last_status: Option<u32>, |
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.
Exit status is defined as an i32
on all platforms
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.
Its technically a u32
on Windows - a DWORD
. While unix is i32
, they are always positive so casting from i32
to u32
seemed like the safest thing to do.
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.
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.
It is but here is the signature for GetExitCodeProcess. Its a pointer to a DWORD.
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.
Ok, no worries - perfectly reasonable!
pub fn new(child: &mut process::Child) -> Child { | ||
let (win_handle, status) = match handle_from_pid(child.id()) { | ||
Some(handle) => (Some(handle), None), | ||
_ => (None, Some(child.wait().unwrap().code().unwrap() as u32)) |
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.
The wait and unwrap here could cause the entire Supervisor process to panic and exit. We need to handle a case where we fail to wait for the child process to exit. tdlr; don't unwrap the wait ;)
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 actually want to panic if it errors here because in that case we have we have lost contact with the process, but we should totally panic with a better error.
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 can't panic the entire process because the Supervisor will be supervising multiple services in the very near future. We can bubble an error up to the top and not handle it for now and that's just fine!
|
||
#[cfg(windows)] | ||
pub use self::windows::{become_command, wait_for_exit, send_signal}; | ||
pub use self::imp::{become_command, send_signal}; |
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.
This line and 33 are duplicates. You do the same re-export if you are Windows or if you aren't Windows ;)
} | ||
|
||
impl HabChild { | ||
pub fn from_child(child: &mut Child) -> HabChild { |
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.
This function is better represented with the From
trait
impl From<&mut Child> for HabChild {
fn from(other: &mut Child) -> HabChild {
HabChild { inner: impl::Child::new(child) }
}
}
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 don't think I can use that trait with a mutable paramerter. correct?
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.
impl From<&mut Child> for HabChild {
yields:
|
45 | impl From<&mut Child> for HabChild {
| ^ expected lifetime parameter
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.
Yeah you totally can, I just didn't tell you the right way to use it ;)
imp<'a> From<&'a mut Child> for HabChild {
fn from(other: &mut Child) -> HabChild {
HabChild { inner: impl::Child::new(child) }
}
}
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.
Turns out because this should now be returning a Result<HabChild>
it no longer satisfies the trait. However I will change the function name to from
pub fn status(&mut self) -> Result<HabExitStatus> { | ||
let mut exit_status: i32 = 0; | ||
|
||
match unsafe { waitpid(self.pid as i32, &mut exit_status, 1 as libc::c_int) } { |
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 should use the libc::WNOHANG
constant here instead of 1 cast as a libc::c_int
unsafe { | ||
match self.status { | ||
None => None, | ||
Some(status) if libc::WIFEXITED(status as i32) => Some(libc::WEXITSTATUS(status as i32) as u32), |
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 should cast to the libc::c_int
type alias of i32
even though this is super unlikely to change ;)
Signed-off-by: Matt Wrock <matt@mattwrock.com>
a2d56d3
to
170d943
Compare
All the comments should now be addressed @reset |
@thesentinels approve |
🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests. |
Travis CI has started testing this PR. |
💖 Travis CI reports this PR passed. It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now. I just want you and the contributor to answer me one question: |
This adds support for windows processes under a supervisor's supervision. Previously when the supervisor checked the status of a running process executed from a supervisor's run hook, we hard coded a
waitpid
status of0
which means there is no transition state for the process and it is running normally. This allowed us to at least run the supervisor and launch run hooks on windows but prevented us from taking any sort of intervention steps.This PR adds a
HabChild
struct that has differentstatus
trait implementatins on linux and windows and both return aHabExitStatus
. Linux runs the familiarwaitpid
and now windows will obtain a processHANDLE
which it can probe for status. In the event that the process dies immediately and there is no handle to obtain, we callwait()
and cache the exit code and return that the next tine the supervisor requests status.This PR also refactors the process status "interpretation" logic formerly used by linux:
supervisor.rs
previously had several internal functions to retrieve exit code or signal. That is now moved to theHabExitStatus
struct and it mostly leverageslibc
for these functions. The logic inHabExitStatus
is modeled after thestd::process::ExitStatus
logic which will return an exit code if the process terminated otherwise it assumes the termination was signaled and it returns the signal.waitpid
(with theWNOHANG
option only), that is not possible.waitpid
returned a pid that was different from the pid being supervised, it would check their signal/exit code. Again, based on the way we are callingwaitpid
(with a specific pid) we will ONLY receive termination status for THAT pid. The intent of this logic appears to have been for reaping zombies and perhaps did just that at one time but I am removing it here since it does not fit well within the new structure and is effectively dead code. That said we may want to pursue a separate fix to address zombies propperly since the current supervisor has no awareness of any process other than the one launched from the run hook.