Eryk Sun <eryk...@gmail.com> added the comment:
> Should there be a `return NULL;` between these two lines? In 3.x, os.kill() has always fallen back on TerminateProcess() when GenerateConsoleCtrlEvent() fails, and I assumed that was intentional. But I misunderstood that it's not actually chaining the exception via PyErr_Fetch and _PyErr_ChainExceptions, so it still fails with a SystemError when both GenerateConsoleCtrlEvent() and TerminateProcess() fail -- e.g. os.kill(4, 1). I checked the history for bpo-1220212. It appears that win32_kill() was committed to the 2.x branch first with a missing return statement, which was fixed a day later: https://github.com/python/cpython/commit/ae509520de5a0321f58c79afffad10ae59dae8b9 A few days later it was ported to 3.x without that fix: https://github.com/python/cpython/commit/eb24d7498f3e34586fee24209f5630a58bb1a04b If Steve Dower or Victor Stinner is okay with changing the behavior to agree with 2.x -- after more than a decade -- then I say just add the missing return statement. In that case, if os.kill() is called with a sig value of CTRL_C_EVENT (0) or CTRL_BREAK_EVENT (1), then it will only try GenerateConsoleCtrlEvent() and no longer fall back on TerminateProcess(). That said, it's common to call TerminateProcess() with an exit status of 1. Maybe the enum values of signal.CTRL_C_EVENT and signal.CTRL_BREAK_EVENT can be checked by identity instead of checking the integer value. That way os.kill(pid, 1) can still be used to forcefully terminate an arbitrary process with an exit status of 1. --- To reiterate previous comments, the sig values of signal.CTRL_C_EVENT and signal.CTRL_BREAK_EVENT are not supported for an individual, arbitrary process. To use them, the pid value must be a process group ID, such as the pid of a process created with the flag CREATE_NEW_PROCESS_GROUP, and the process must be in the same console session as the current process. Anything else either fails (assuming no fallback on TerminateProcess) or invokes buggy behavior in the console, which can even leave the console in a broken state. For a new process group, the cancel event is initially ignored, but the break event is always handled. To enable the cancel event, the process must call SetConsoleCtrlHandler(NULL, FALSE), such as via ctypes with kernel32.SetConsoleCtrlHandler(None, False). I think the signal module should provide a function to enable/disable Ctrl+C handling without ctypes, and implicitly enable it when setting a new SIGINT handler. --- Hypothetical la la land... I wish os.kill() in Windows had been implemented to conform with Unix kill(), and that CTRL_C_EVENT and CTRL_BREAK_EVENT were not added since they are not signals. Route negative pid values to the process-group branch that calls GenerateConsoleCtrlEvent(), and special case pid -1 to send the event to all accessible processes in the console session. Support SIGINT and SIGBREAK sig values in this case, respectively mapped to the console's cancel and break events. Don't support pid 0 in Windows, since there's no documented function to get the process group ID of the current process. Route positive pid values to the TerminateProcess() branch. Special case the sig value 0 to test for existence and access via OpenProcess(). Also, the latter should only request PROCESS_TERMINATE access, not PROCESS_ALL_ACCESS. It's common to have terminate access but not all access, e.g. to an elevated process in the current desktop session. ---------- components: +Library (Lib) -Extension Modules _______________________________________ Python tracker <rep...@bugs.python.org> <https://bugs.python.org/issue42962> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
