Skip to content

Behaviour of os.kill() on Windows may be misunderstood #125298

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

Closed
honorasm opened this issue Oct 11, 2024 · 3 comments
Closed

Behaviour of os.kill() on Windows may be misunderstood #125298

honorasm opened this issue Oct 11, 2024 · 3 comments
Labels
docs Documentation in the Doc dir OS-windows

Comments

@honorasm
Copy link

honorasm commented Oct 11, 2024

The latest os.kill() document says “The Windows version of kill() additionally takes process handles to be killed,” but the kill() function actually does not accept process handles instead of process IDs. Runing the following code with Python 3.12.6 on Windows 10 Education Edition version 22H2 amd64 will result in an error.

Test code:

import os, signal
proc_handle = os.spawnl(os.P_NOWAIT, r'C:\Windows\notepad.exe', 'notepad.exe')
os.kill(proc_handle, signal.SIGTERM)

Error message:

Traceback (most recent call last):
  File "C:\Users\John\Documents\Python\os_kill_proc_handle.py", line 3, in <module>
    os.kill(proc_handle, signal.SIGTERM)
OSError: [WinError 87] The parameter is incorrect

Linked PRs

@honorasm honorasm added the docs Documentation in the Doc dir label Oct 11, 2024
@picnixz
Copy link
Member

picnixz commented Oct 11, 2024

I think this is a doc imprecision. What os.kill() does is:

cpython/Modules/posixmodule.c

Lines 9528 to 9559 in 08f6bf7

#ifdef HAVE_WINDOWS_CONSOLE_IO
/* Console processes which share a common console can be sent CTRL+C or
CTRL+BREAK events, provided they handle said events. */
if (sig == CTRL_C_EVENT || sig == CTRL_BREAK_EVENT) {
if (GenerateConsoleCtrlEvent(sig, (DWORD)pid) == 0) {
err = GetLastError();
PyErr_SetFromWindowsErr(err);
}
else {
Py_RETURN_NONE;
}
}
#endif /* HAVE_WINDOWS_CONSOLE_IO */
/* If the signal is outside of what GenerateConsoleCtrlEvent can use,
attempt to open and terminate the process. */
handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE, (DWORD)pid);
if (handle == NULL) {
err = GetLastError();
return PyErr_SetFromWindowsErr(err);
}
if (TerminateProcess(handle, sig) == 0) {
err = GetLastError();
result = PyErr_SetFromWindowsErr(err);
} else {
result = Py_NewRef(Py_None);
}
CloseHandle(handle);
return result;
#endif /* !MS_WINDOWS */

So, it only expect PIDs but may try to use process handles instead in order to kill the desired PID. I am no Windows expert but that's how I understand it. Now, I think we could perhaps improve the wording though I don't have a good suggestion.

cc @zooba as a Windows expert

@picnixz picnixz changed the title Error in os.kill() Document Behaviour of os.kill() on Windows may be misunderstood Oct 11, 2024
@csm10495
Copy link
Contributor

It kind of seems like

The Windows version of kill() additionally takes process handles to be killed.

Could be removed for clarity. If something must be stated about the implementation, maybe:

On Windows, the pid is mapped to a process handle which is then passed along with the signal to TerminateProcess.

@rruuaanng
Copy link
Contributor

rruuaanng commented Oct 20, 2024

Actually this is a documentation issue, since TerminateProcess only receives a handle, it needs to get the pid, but the documentation doesn't really need to state this, since this is implementation-level and not user-facing. It could be removed, and there's no need to add extra documentation to describe this behavior.
Edit:
Although in Windows, handles and pid are almost identical, this may seem strange to unfamiliar users.

zooba pushed a commit that referenced this issue Nov 8, 2024
Windows has not accepted process handles in many releases.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 8, 2024
…honGH-125749)

Windows has not accepted process handles in many releases.
(cherry picked from commit 75ffac2)

Co-authored-by: RUANG (James Roy) <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 8, 2024
…honGH-125749)

Windows has not accepted process handles in many releases.
(cherry picked from commit 75ffac2)

Co-authored-by: RUANG (James Roy) <[email protected]>
zooba pushed a commit that referenced this issue Nov 8, 2024
Windows has not accepted process handles in many releases.
(cherry picked from commit 75ffac2)

Co-authored-by: RUANG (James Roy) <[email protected]>
zooba pushed a commit that referenced this issue Nov 8, 2024
Windows has not accepted process handles in many releases.
(cherry picked from commit 75ffac2)

Co-authored-by: RUANG (James Roy) <[email protected]>
@zooba zooba closed this as completed Nov 8, 2024
picnixz pushed a commit to picnixz/cpython that referenced this issue Dec 8, 2024
…honGH-125749)

Windows has not accepted process handles in many releases.
ebonnal pushed a commit to ebonnal/cpython that referenced this issue Jan 12, 2025
…honGH-125749)

Windows has not accepted process handles in many releases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir OS-windows
Projects
Status: Todo
Development

No branches or pull requests

5 participants