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

os.kill on windows #42083

Closed
tebeka mannequin opened this issue Jun 14, 2005 · 30 comments
Closed

os.kill on windows #42083

tebeka mannequin opened this issue Jun 14, 2005 · 30 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@tebeka
Copy link
Mannequin

tebeka mannequin commented Jun 14, 2005

BPO 1220212
Nosy @tim-one, @loewis, @tebeka, @tiran, @tpn, @DinoV, @voidspace, @briancurtin
Files
  • diff.zip: Zip of source and documentation
  • kill.diff
  • issue1220212.patch
  • faq_update.diff
  • 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 = 'https://github.com/briancurtin'
    closed_at = <Date 2010-04-12.18:11:07.992>
    created_at = <Date 2005-06-14.09:11:53.000>
    labels = ['interpreter-core', 'type-feature']
    title = 'os.kill on windows'
    updated_at = <Date 2010-04-12.18:11:07.991>
    user = 'https://github.com/tebeka'

    bugs.python.org fields:

    activity = <Date 2010-04-12.18:11:07.991>
    actor = 'brian.curtin'
    assignee = 'brian.curtin'
    closed = True
    closed_date = <Date 2010-04-12.18:11:07.992>
    closer = 'brian.curtin'
    components = ['Interpreter Core']
    creation = <Date 2005-06-14.09:11:53.000>
    creator = 'tebeka'
    dependencies = []
    files = ['6690', '6691', '16739', '16765']
    hgrepos = []
    issue_num = 1220212
    keywords = ['patch']
    message_count = 30.0
    messages = ['48455', '48456', '48457', '48458', '48459', '48460', '48461', '48462', '48463', '48464', '59787', '59800', '62468', '101774', '101775', '101777', '101828', '101846', '101847', '101851', '101970', '101973', '102184', '102200', '102202', '102213', '102375', '102379', '102385', '102965']
    nosy_count = 13.0
    nosy_names = ['tim.peters', 'loewis', 'nnorwitz', 'tebeka', 'andersjm', 'techtonik', 'christian.heimes', 'kap4020', 'Technologov', 'trent', 'dino.viehland', 'michael.foord', 'brian.curtin']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1220212'
    versions = ['Python 2.7', 'Python 3.2']

    @tebeka
    Copy link
    Mannequin Author

    tebeka mannequin commented Jun 14, 2005

    This patch enables os.kill on windows. This way there
    will be an "out of the box" way to kill process on windows.

    Basically kill calls TerminateProcess and the "signal"
    paramer will be the process return code.

    Don't have CVS access today so I'm sending the whole
    files in a zip (sorry).

    Also some config wizard need to change the winsows
    configuration and set HAVE_KILL.

    @tebeka tebeka mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jun 14, 2005
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 11, 2005

    Logged In: YES
    user_id=21627

    Converted to unified diff.

    @kap4020
    Copy link
    Mannequin

    kap4020 mannequin commented Jul 6, 2006

    Logged In: YES
    user_id=1537118

    So is there a reason this isn't in the latest Python? The
    patch is tiny.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Jul 6, 2006

    Logged In: YES
    user_id=33168

    Karl, can you test this patch and verify it works? It would
    be especially helpful to test on Win9x and WinNT variants.
    Are there tests included? Is there a doc patch? These are
    some of the potential reasons this isn't included. As for
    me, I can't test this patch (no windows), so it's a
    non-starter. If you'd like to help out, we could use the help.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 6, 2006

    Logged In: YES
    user_id=21627

    As is, the patch has two major problems:

    • TerminateProcess expects a process handle, not a process
      ID. This makes the patch pretty useless: To kill a process
      other than yourself, you need to call OpenProcess first (or
      obtain a process handle in some other way, e.g. by having
      created it).

    • if the call fails, it uses posix_error to report the
      problem. However, posix_error expects that errno is set,
      which isn't the case here.

    Furthermore, this patch would duplicate
    _subprocess.TerminateProcess, which already exposes
    TerminateProcess (plus allowing to specify the exit code).

    So in its current form, I think the patch should be rejected.

    @tebeka
    Copy link
    Mannequin Author

    tebeka mannequin commented Jul 9, 2006

    Logged In: YES
    user_id=358087

    IIRC (the patch was done a long time ago), Python uses the
    process handle as the process id, I've tested the code and
    it worked.

    I agree that the patch need more work, mainly the error
    return value.

    At least on Python 2.4.3, TerminateProcess is not exposed by
    _subprocss (or subprocess).

    I still think that having a way to kill a process OOTB on
    win32 is very important, pretty swamped right now but I'll
    try to improve the patch.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 9, 2006

    Logged In: YES
    user_id=21627

    "Python uses the process handle as process id" makes no real
    sense - Win32 works differently. You can't terminate a
    process without a handle, and you can't just "use" the
    process id as the handle. Perhaps some use cases for the API
    would need to be defined first.

    @tim-one
    Copy link
    Member

    tim-one commented Jul 9, 2006

    Logged In: YES
    user_id=31435

    Martin, FYI, on Windows the os.spawn() functions return the
    process handle, so that's the natural use case: killing a
    process spawned by Python's os.spawn*(). This is a little
    confusing because the spawn() functions are documented as
    returning "the process ID", and verbiage inside the spawn
    docs explains that it doesn't really mean "process ID" on
    Windows.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 9, 2006

    Logged In: YES
    user_id=21627

    Ah, ok. Given that _subprocess already offers that
    functionality, I'm still -1 on adding it to "nt". Adding
    some kind of OpenProcess might be more useful, but then,
    people can get to all these functions through ctypes, as well.

    @tim-one
    Copy link
    Member

    tim-one commented Jul 9, 2006

    Logged In: YES
    user_id=31435

    Well, os.waitpid() on Windows also takes a process handle.
    This was (of course) deliberate, so that os.waitpid() could
    use the thingie returned by os.spawn() on Windows. If
    os.kill() were ever added to Windows, I think it would be
    quite natural for it to take a process handle too, and for
    the same reason (Python code mixing os.{spawn,waitpid,kill}
    could be the same across platforms). Zope's ZODB relies on
    mixing spawn and waitpid this way, and Zope's ZRS implements
    its own kill function on Windows so that the rest of the
    Python code can just do kill(id) on all platforms (where
    id is always something obtained from os.spawn()).

    @tiran
    Copy link
    Member

    tiran commented Jan 12, 2008

    This should be implemented differently. Users should substitute their
    popen and exec calls with subprocess. The subprocess.Popen class should
    gain two new methods terminate() and send_signal(int) where send_signal
    is restricted to SIGKILL (+SIGTERM ?) on Windows. The idea was discussed
    on the Python dev list early last year.

    @tebeka
    Copy link
    Mannequin Author

    tebeka mannequin commented Jan 12, 2008

    Users should substitute their popen and exec calls with subprocess
    As long as popen and exec are available, users are free to use them (and
    probably will :)

    The Popen(...).terminate() works only if I'm the one who started the
    process. However there are cases where I want to kill all running python
    processes. The pids to be killed will come from somewhere else (ps,
    plist, ...).

    @Technologov
    Copy link
    Mannequin

    Technologov mannequin commented Feb 16, 2008

    Yes, this feature would be very important for me too...

    Anybody knows, _when_ it will be integrated into Python ?

    If this bugzilla supports email notifications, Please add me as "CC" for
    this bug.

    -Technologov

    @voidspace
    Copy link
    Contributor

    It would be really useful to be able to send signal.SIGINT to processes on Windows using os.kill(...). The patch as described sounds like it would have a different signature to the standard implementation of os.kill(...) which takes a pid and a signal type.

    IronPython 2.7 will have an os.kill implementation. Looks like it only supports signal.SIGINT and signal.SIGBREAK and just calls:

    Process toKill = Process.GetProcessById(pid);
    toKill.Kill()
    

    @voidspace
    Copy link
    Contributor

    I'm wrong. First IronPython tries:

    NativeSignal.GenerateConsoleCtrlEvent(PythonSignal.CTRL_C_EVENT, ...)

    But with the comment:

    //The following calls to GenerateConsoleCtrlEvent will fail under
    //most circumstances. We'll try them any ways though as this seems
    //to be the only mechanism in Windows to send signals to processes

    It falls back to just killing the process.

    @voidspace
    Copy link
    Contributor

    Aaaand the IronPython implementation of NativeSignal.GenerateConsoleCtrlEvent(PythonSignal.CTRL_C_EVENT, ...)
    delegates to Kernel32.GenerateConsoleCtrlEvent.

    @voidspace
    Copy link
    Contributor

    To make it clear, even though it would be incomplete, a partial implementation of os.kill(...) for Windows would be very useful and provide some level of compatibility with applications that use os.kill (so even if os.kill(...) duplicates functionality in other modules - although that was disputed - it should be provided for compatibility reasons).

    An implementation similar to the IronPython one is probably the best that can be managed and would still be useful: accepting only signal.SIGINT and signal.SIGBREAK and first trying Kernel32.GenerateConsoleCtrlEvent, falling back to killing the process.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Mar 27, 2010

    So, Michael, what do you think: should os.kill expect a process handle, or a process ID?

    @voidspace
    Copy link
    Contributor

    Hmm... well my particular use case is that it should work with the value returned by os.getpid(). If that is a process handle then it is nice and convenient to just use process handles. The docs don't specify so I bet it returns a pid. :-(

    However Brian Curtin might want to weigh in on this as he has been looking at implementing this and there might be constraints.

    @briancurtin
    Copy link
    Member

    I have this working with process IDs and my vote would be to keep it that way, as it would stay in-line with the other platforms, and it seems to work so far. I would imagine that was also IronPython's goal.

    I'm still working on the test portion of the patch. It should be ready shortly.

    @briancurtin
    Copy link
    Member

    Here is a patch with some tests and doc changes.

    I'm having trouble coming up with tests which will work with CTRL_C_EVENT and CTRL_BREAK_EVENT. Based on my understanding of GenerateConsoleCtrlEvent, I figured this example (http://msdn.microsoft.com/en-us/library/ms685049%28v=VS.85%29.aspx) would be receptive to CTRL+C when started as a subprocess, but it doesn't work. I also figured "python -c "input()"" started as a subprocess could be killed with CTRL_C_EVENT, but it doesn't work either.

    @briancurtin
    Copy link
    Member

    Removed an unnecessary goto and fixed a few tab/space inconsistencies (ugh).

    @voidspace
    Copy link
    Contributor

    After discussion with Brian it seems like it should be possible for os.kill(...) on Windows to support both pids *and* process handles. This seems ideal.

    @briancurtin
    Copy link
    Member

    Michael, do you have an example of something which returns a handle? This current patch doesn't work with handles, but it wouldn't be hard to add it. I could make it work with the _handle object of a Popen object, but you could just as easily call os.kill on the pid of the Popen object. I don't know of any Python-wide handle object...I know subprocess has it's own, and don't really know what other functions are returning handles.

    Anyways, this patch includes an additional test script which uses ctypes to setup a console control handler, and CTRL_BREAK_EVENT is tested successfully. However, CTRL_C_EVENT is not. See http://msdn.microsoft.com/en-us/library/ms686016%28v=VS.85%29.aspx for details, but I'm not able to get the subprocess to work with CTRL+C. Calling SetConsoleCtrlHandler(NULL, FALSE) either in the script, test_os, or in _subprocess.c does not change anything, although it seems that it should. The CTRL_C_EVENT test is currently skipped in the patch until I can figure that out.

    @voidspace
    Copy link
    Contributor

    According to earlier discussion in this issue os.spawn() return process handles on Windows.

    @briancurtin
    Copy link
    Member

    Committed to trunk in r79633 after talking with Michael about it. I'll forward port it after the 2.7 beta goes out.

    @briancurtin briancurtin self-assigned this Apr 2, 2010
    @briancurtin briancurtin added the type-feature A feature request or enhancement label Apr 2, 2010
    @briancurtin briancurtin self-assigned this Apr 2, 2010
    @briancurtin briancurtin added the type-feature A feature request or enhancement label Apr 2, 2010
    @techtonik
    Copy link
    Mannequin

    techtonik mannequin commented Apr 5, 2010

    @briancurtin
    Copy link
    Member

    How about something like this patch?

    @techtonik
    Copy link
    Mannequin

    techtonik mannequin commented Apr 5, 2010

    Seems good to me, even though I'd rewrite some parts like this:

    • Prior to Python 2.7 and 3.2, to terminate a process, you can use ctypes::
      + Prior to Python 2.7 and 3.2, you can use linksomehow:`ctypes` to
      terminate a process::

    ...

    In newer Python versions :func:`os.kill` works on Windows with the
    additional feature of being able to send CTRL+C and CTRL+BREAK to
    console subprocesses that understand these signals.

    ...

    anatoly t.

    On Mon, Apr 5, 2010 at 6:38 PM, Brian Curtin <report@bugs.python.org> wrote:

    Brian Curtin <curtin@acm.org> added the comment:

    How about something like this patch?

    ----------
    Added file: http://bugs.python.org/file16765/faq_update.diff


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue1220212\>


    @briancurtin
    Copy link
    Member

    Ported to py3k in r80008.

    FAQ text updated in r80009 and r80010.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 9, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants