Skip to content
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

isalive(): check process status for non-child process #37

Open
masteret opened this issue Mar 20, 2017 · 13 comments
Open

isalive(): check process status for non-child process #37

masteret opened this issue Mar 20, 2017 · 13 comments

Comments

@masteret
Copy link

The original implementation of isalive uses os.waitpid to check if the ptyprocess is alive
os.waitpid can only check the process status if that process is a child of the current process

If the pexpect object is passed from process A to process B
process B can access the pexpect object but it cannot use it because os.waitpid can't find the ptyprocess under the child list of process B
it will throw the no such process error
while the process actually exists and is still running

To solve this
It can use os.kill to send signal 0 to check if process is alive
which achieve the same functionality of os.waitpid but not limit it to child process

Is it possible to add this as an extra check for isalive()?

@masteret
Copy link
Author

I have made a commit in my fork repo of ptyprocess
masteret@420f657
If it is ok to use please allow me to create a PR

@rocmit
Copy link

rocmit commented May 2, 2017

I also ran into this issue. It would be great if this issue can be fixed soon!

@rocmit
Copy link

rocmit commented May 5, 2017

Just wanted to point out another thing that we should consider when implementing the solution...

In the same scenario, if the child is closed, terminated, or killed by process B, it will become a zombie process (PID is still valid but the process is dead)... This os.kill(pid, 0) solution would not catch that since the return would be None, instead of an exception.

The use case is like this: 1) Process A created an child session 2) Process B is created by multiprocess and it uses the same child session 3) If Process B kills the child session that was created under process A, os.kill(pid, 0) will return None and isalive will return True.

@MountainRider
Copy link

MountainRider commented May 6, 2017

Won't Process A successfully call os.waitpid(self.pid, waitpid_options) on the child?

@rocmit
Copy link

rocmit commented May 7, 2017

Yes, process A would be able to get the correct isalive() status with os.waitpid. However, if process B runs isalive check, it would run into the situation I described.

@MountainRider
Copy link

I just verified this with some code. After Process B killed Process C, os.kill did not raise an exception.

@masteret
Copy link
Author

masteret commented May 9, 2017

Thank you for the comments, I will look into the scenario and see if I can check it correctly

@MountainRider
Copy link

Perhaps @rocmit can make an helpful suggestion.

@masteret
Copy link
Author

Since process A can get a correct isalive() status with os.waitpid
The problem only happens when non-parent process call isalive with ptyprocess pid
I can add an additional check to see if the pid is a zombie process by looking at the 3rd item in /proc/${pid}/stat
if it is Z then that means the process is a zombie, then we can assume it is not alive

@MountainRider
Copy link

There won't be a proc file system on OS X.

@rocmit
Copy link

rocmit commented May 10, 2017

/proc/pid/stat file is what I use to workaround this after doing some research. Like MountainRider mentioned, this may not work on other OS.

@masteret
Copy link
Author

After some research, I found psutil has a way to determine if a proc is zombie in different os.
In Linux, it checks the proc and in OSX it uses sysctl in C.

@jquast
Copy link
Member

jquast commented May 10, 2017

Yes, a maintainer here to chime in, the only thing preventing me from reviewing this PR quickly is to get FreeBSD, Linux, and mac machines to run the current tests--we may even need additional tests to cover new logic here, we'll see what the coverage report says, and/ or write some if-else conditions around platform if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants