missing return in win32_kill?

@pitrou

BPO	14484
Nosy	@pitrou, @vstinner, @tjguk, @bitdancer, @asvetlov, @zware, @eryksun, @zooba, @iritkatriel

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2012-04-03.14:10:25.629>
labels = ['3.10', 'type-bug', '3.9', 'OS-windows', '3.11']
title = 'missing return in win32_kill?'
updated_at = <Date 2021-12-13.22:46:11.595>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2021-12-13.22:46:11.595>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Windows']
creation = <Date 2012-04-03.14:10:25.629>
creator = 'pitrou'
dependencies = []
files = []
hgrepos = []
issue_num = 14484
keywords = []
message_count = 20.0
messages = ['157419', '157421', '157422', '157424', '157426', '157428', '157429', '222918', '223684', '223739', '236320', '240809', '240815', '240819', '240824', '240912', '408328', '408337', '408454', '408487']
nosy_count = 10.0
nosy_names = ['jpe', 'pitrou', 'vstinner', 'tim.golden', 'r.david.murray', 'asvetlov', 'zach.ware', 'eryksun', 'steve.dower', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14484'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

Here is an excerpt from the os.kill implementation under Windows (in win32_kill(), posixmodule.c):

    if (sig == CTRL_C_EVENT || sig == CTRL_BREAK_EVENT) {
        if (GenerateConsoleCtrlEvent(sig, pid) == 0) {
            err = GetLastError();
            PyErr_SetFromWindowsErr(err);
        }
        else
            Py_RETURN_NONE;
    }

It seems there is a missing return in the first branch, when GenerateConsoleCtrlEvent() fails.

Antonie, you right.

I can't find where we talked about this, maybe just IRC, but that's there (perhaps poorly) as a special case.

0 is 0, but signal.CTRL_C_EVENT is also 0. We try the signal version first then fall back to TerminateProcess.

I was just looking at this myself and it's not great. I actually wish we could change what signal.CTRL_C_EVENT means, or maybe add a flag to determine if the second parameter is supposed to be a return code or a signal? I'm open to anything.

I can't find where we talked about this, maybe just IRC, but that's
there (perhaps poorly) as a special case.

0 is 0, but signal.CTRL_C_EVENT is also 0. We try the signal version
first then fall back to TerminateProcess.

Then why set the error?
That said, abruptly killing another process and making it return 0
sounds a bit perverted.

I don't remember exactly why, but it can be removed. It may have just been left in while I was debugging it.

As for the second point, why else are you calling os.kill if you don't want to kill the given process? I don't disagree that it's on the perverse side, but that's the functionality available. Perhaps we should *not* have the fallback and only operate on the signals?

As for the second point, why else are you calling os.kill if you don't
want to kill the given process? I don't disagree that it's on the
perverse side, but that's the functionality available. Perhaps we
should *not* have the fallback and only operate on the signals?

I just meant that exiting with 0 isn't exactly expected when a process
is forcibly killed (0 meaning success, as far as I know).

I would think that if Windows doesn't support a specific signal, os.kill should raise a ValueError. But I'm an outsider here, I know nothing about how Windows works for this except what I'm reading here.

To answer your question: there are many reasons to call kill on unix, and only a few of them kill the process. Kill is just an historical name, it really means 'send a signal'.

In a broader picture, I think that os.kill calls should have the same "meaning", insofar as possible, on both linux and windows. Having a single API with the same syntax but different semantics on different platforms sounds bad to me.

@zach

@zach can you add anything to this?

It looks like we have a bit of a mess here. 2.7 has a return there (and thus doesn't fall back to TerminateProcess if GenerateConsoleCtrlEvent fails), added 40 commits after the initial implementation in b1c00c7d3c85, but 3.x was never changed so 2.7 and 3.x have behaved differently from the the time it was implemented. Which version is right, or is it too late to change either one and 3.x should just remove the unused error setting? An interesting possibility might be to convert the signal.CTRL_* values to an enum, and use that as a way to distinguish between 0 and signal.CTRL_C_EVENT. I suspect that might become rather hairy, though.

Either way, I don't think os.kill can promise much more than "try to make the specified process die" on Windows; signals are just so crippled on Windows that that's about all you can do with them anyway. It might not hurt for the docs to try to make that clearer.

I understand that os.kill(pid, sig) should call TerminateProcess() or GenerateConsoleCtrlEvent() depending on the value of sig.

The problem is that these two functions are very different. A process can set a control handler for CTRL_C_EVENT and CTRL_BREAK_EVENT, so can decide how to handle GenerateConsoleCtrlEvent() event.

TerminateProcess() kills the process with the specified exit code.

To me it looks wrong to call TerminateProcess() with a signal number or event for the exit code!? We need to expose TerminateProcess() as a new Python function, os.TerminateProcess(pid, exitcode) for example.

os.kill(pid, sig) should raise a ValueError if sig is not CTRL_C_EVENT nor CTRL_BREAK_EVENT.

bpo-14480 "os.kill on Windows should accept zero as signal" references this. It seems that we either go all the way and change the code as Victor has suggested or keep the status quo and change the docs as Zach has said. Thoughts?

I think at a minimum, a return should be added in the cases that call GenerateConsoleCtrlEvent and it fails.

Here's a more radical proposal, though: deprecate kill() on Windows and add a function that calls GenerateConsoleCtrlEvent and another that calls TerminateProcess. The rationale is that the two do act quite a bit differently than kill does on non-Windows systems do and it's a bad idea to try to provide cross-platform functionality when it can't be done. kill() on non-Windows systems would be left alone.

I think what I'd rather do is deprecate the use of os.kill (on Windows) with a signal other than CTRL_C_EVENT and CTRL_BREAK_EVENT, document very clearly that os.kill is strictly for sending signals, and add os.terminate to implement TerminateProcess.

Windows and LogMonitor (#30643)

Windows and LogMonitor (ray-project#30643)

Happy New Year, everyone! It's now 2025. Let's revisit the issue mentioned here. Should we start by adding a missing return? I have reviewed the current code, and it typically handles errors as follows:

    if (xxx == NULL) {
        err = GetLastError();
        return PyErr_SetFromWindowsErr(err);
    }

or

    if (xxx == 0) {
        err = GetLastError();
        return PyErr_SetFromWindowsErr(err);
    }

diff:

     if (sig == CTRL_C_EVENT || sig == CTRL_BREAK_EVENT) {
         if (GenerateConsoleCtrlEvent(sig, (DWORD)pid) == 0) {
             err = GetLastError();
-            PyErr_SetFromWindowsErr(err);
+            return PyErr_SetFromWindowsErr(err);
         }
         else {
+            /* The generated signal has already been sent to the current console. */
             Py_RETURN_NONE;
         }
     }

pythongh-58689: Fix os.kill() error handling on Windows

Oh, this issue is still open in 2025? :-) I wrote a quick fix for it: PR gh-128932.

gh-58689: Fix os.kill() error handling on Windows (#128932)

pythongh-58689: Fix os.kill() error handling on Windows (pythonGH-128932

Windows os.kill() error handling was fixed by the change 939df0f.

I don't think that we can change os.kill() behavior on Windows. It's now too late :-( We should stick to the status quo. I close the issue.

Other issues (linked here) describes the behavior and how it could be changed.

[3.12] gh-58689: Fix os.kill() error handling on Windows (GH-128932) (#…

[3.13] gh-58689: Fix os.kill() error handling on Windows (GH-128932) (#…

pythongh-58689: Fix os.kill() error handling on Windows (python#128932)

Navigation Menu

Repository navigation

missing return in win32_kill? #58689

Linked PRs

18 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

missing return in win32_kill? #58689

Description

Linked PRs

Activity

pitrou commented on Apr 3, 2012

asvetlov commented on Apr 3, 2012

briancurtin commented on Apr 3, 2012

pitrou commented on Apr 3, 2012

briancurtin commented on Apr 3, 2012

pitrou commented on Apr 3, 2012

bitdancer commented on Apr 3, 2012

BreamoreBoy commented on Jul 13, 2014

zware commented on Jul 22, 2014

vstinner commented on Jul 23, 2014

BreamoreBoy commented on Feb 20, 2015

jpe commented on Apr 13, 2015

zware commented on Apr 14, 2015

18 remaining items

rruuaanng commented on Jan 16, 2025

vstinner commented on Jan 17, 2025

vstinner commented on Jan 17, 2025

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions