So this is kind of a big one and the repercussions are hard to describe because it's subtle. This test case:
| def assert_gone(pid): | |
| # XXX | |
| if WINDOWS: | |
| return | |
| assert not psutil.pid_exists(pid), pid | |
| assert pid not in psutil.pids(), pid | |
| try: | |
| p = psutil.Process(pid) | |
| assert not p.is_running(), pid | |
| except psutil.NoSuchProcess: | |
| pass | |
| else: | |
| assert 0, "pid %s is not gone" % pid |
...which was disabled on Windows, shows that processes unexpectedly stick around after being terminate()d and wait()ed on. The problem originates from OpenProcess() Windows API.
OpenProcess() is unreliable in that it can return a handle for a process PID which no longer exists, so we assume that GetExitCodeProcess returning STILL_ACTIVE means it's running, else it isn't. It turns out this is not always the case. This had repercussions on psutil.pid_exists() API which returned an incorrect value, and also on other psutil APIs relying on it internally (cmdline(), environ(), cwd(), connections()). This PR fixes this logic and assumes a process handle is actually running if:
OpenProcessdoesn't return ERROR_INVALID_PARAMETERGetExitCodeProcessmust returnSTILL_ACTIVE- if not then iterate over all PIDs, which is slower but safe (this is the part which was missing)
Full logic here:
/* * Given a process HANDLE checks whether it's actually running. * Returns: * - 1: running * - 0: not running * - -1: WindowsError * - -2: AssertionError */ int psutil_is_phandle_running(HANDLE hProcess, DWORD pid) { DWORD processExitCode = 0; if (hProcess == NULL) { if (GetLastError() == ERROR_INVALID_PARAMETER) { // Yeah, this is the actual error code in case of // "no such process". if (! psutil_assert_pid_not_exists( pid, "iphr: OpenProcess() -> ERROR_INVALID_PARAMETER")) { return -2; } return 0; } return -1; } if (GetExitCodeProcess(hProcess, &processExitCode)) { // XXX - maybe STILL_ACTIVE is not fully reliable as per: // http://stackoverflow.com/questions/1591342/#comment47830782_1591379 if (processExitCode == STILL_ACTIVE) { if (! psutil_assert_pid_exists( pid, "iphr: GetExitCodeProcess() -> STILL_ACTIVE")) { return -2; } return 1; } else { // We can't be sure so we look into pids. if (psutil_pid_in_pids(pid) == 1) { return 1; } else { CloseHandle(hProcess); return 0; } } } CloseHandle(hProcess); if (! psutil_assert_pid_not_exists( pid, "iphr: exit fun")) { return -2; } return -1; }
The logic of pid_exists() changed in an almost identical manner:
| int | |
| psutil_pid_is_running(DWORD pid) { | |
| HANDLE hProcess; | |
| DWORD exitCode; | |
| DWORD err; | |
| // Special case for PID 0 System Idle Process | |
| if (pid == 0) | |
| return 1; | |
| if (pid < 0) | |
| return 0; | |
| hProcess = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, | |
| FALSE, pid); | |
| if (NULL == hProcess) { | |
| err = GetLastError(); | |
| // Yeah, this is the actual error code in case of "no such process". | |
| if (err == ERROR_INVALID_PARAMETER) { | |
| if (! psutil_assert_pid_not_exists( | |
| pid, "pir: OpenProcess() -> INVALID_PARAMETER")) { | |
| return -1; | |
| } | |
| return 0; | |
| } | |
| // Access denied obviously means there's a process to deny access to. | |
| else if (err == ERROR_ACCESS_DENIED) { | |
| if (! psutil_assert_pid_exists( | |
| pid, "pir: OpenProcess() ACCESS_DENIED")) { | |
| return -1; | |
| } | |
| return 1; | |
| } | |
| // Be strict and raise an exception; the caller is supposed | |
| // to take -1 into account. | |
| else { | |
| PyErr_SetFromWindowsErr(err); | |
| return -1; | |
| } | |
| } | |
| if (GetExitCodeProcess(hProcess, &exitCode)) { | |
| CloseHandle(hProcess); | |
| // XXX - maybe STILL_ACTIVE is not fully reliable as per: | |
| // http://stackoverflow.com/questions/1591342/#comment47830782_1591379 | |
| if (exitCode == STILL_ACTIVE) { | |
| if (! psutil_assert_pid_exists( | |
| pid, "pir: GetExitCodeProcess() -> STILL_ACTIVE")) { | |
| return -1; | |
| } | |
| return 1; | |
| } | |
| // We can't be sure so we look into pids. | |
| else { | |
| return psutil_pid_in_pids(pid); | |
| } | |
| } | |
| else { | |
| err = GetLastError(); | |
| CloseHandle(hProcess); | |
| // Same as for OpenProcess, assume access denied means there's | |
| // a process to deny access to. | |
| if (err == ERROR_ACCESS_DENIED) { | |
| if (! psutil_assert_pid_exists( | |
| pid, "pir: GetExitCodeProcess() -> ERROR_ACCESS_DENIED")) { | |
| return -1; | |
| } | |
| return 1; | |
| } | |
| else { | |
| PyErr_SetFromWindowsErr(0); | |
| return -1; | |
| } | |
| } | |
| } |