Perhaps you need to test that cleanup_timeout is respected here :)

Oof. Now I've fixed the copy/paste mistake.

Close the file to ensure the output is flushed

I thought, in CPython at least, that the file is guaranteed to be closed during __del__. But you're right: "explicit is better than implicit". :-) Now fixed.

Ex apparently not used

Nice catch. Removed.

Thread is not joined, so could leak to other tests

ok, now joined.

Current implementation looks like it will leave the child running, rather than killing it. Opportunity for another test case :)

Ouch, that's embarrassing. Thanks for catching it. I've pushed a new commit which fixes that... with a test case. ;-)

Is it important to change the exception here? If so, why make a new KeyboardInterrupt exception rather than reraising the original one?

Is it important to change the exception here?

Yes, definitely. The user hit Ctrl+C, so the expected behavior is to raise KeyboardInterrupt.

If so, why make a new KeyboardInterrupt exception rather than reraising the original one?

Yeah, I asked myself this question, too. I figured this was the clearest thing to write, and I also like the fact that, thanks to Python 3's automatic exception chaining, power-user could read the traceback carefully and see that the subprocess cleanup code timed out.

Anyway, I don't feel very strongly about it. Open to suggestions.

Cleanup_timeout is the additional duration in seconds. What objects are supported: int, float, any real number?

Please remove the print().

Oops! Done.

I dislike a default timeout of 1 second. We should not hardcode arbitrary delay in the subprocess module, it's too hard to guess the "best" value. I prefer to keep the current behaviour if timeout is not set: kill immediately the process.

I dislike the idea of having a special case only for KeyboardInterrupt. I would prefer to put this code in the bare "except:" case.

I propose to close stdin, stdout and stderr here to notify the child process that we want it to stop.

Otherwise, the program can hang on writing into stdout if the stdout pipe is full.

I would prefer to call "raise" in a new "else:" block.

Hum, I dislike playing with process groups in an unit test. I would prefer to only send SIGINT directly to processes using their pid.

You cannot assume that a child will get SIGINT, this is true only for a foreground process in the shell. The parent process can be a daemon and nobody will generate SIGINT for its children.

(Disclaimer: I cannot claim to be an expert in POSIX conventions, so forgive me if you have expertise in this area and know the following to be incorrect.)

If I understand correctly, the primary distinction betweenSIGTERM and SIGINT is that the latter is specifically intended to be used in interactive terminal programs. For instance, that's why a POSIX-compliant shell automatically forwards SIGINT to all processes in the foreground group. (The same goes for the windows equivalent, CTRL_C_EVENT.) Of course, that's also why Python chose to name it's SIGINT-triggered exception KeyboardInterrupt.

It is reasonable to assume that receiving aSIGINT is a strong clue to the program that it is running in the foreground process group, and therefore its child is, too.

IMHO, programmers who would send a SIGINT programmatically shouldn't be surprised by slightly odd behavior. But the good news is that the "odd behavior" in this case is quite benign: it's merely a 1 second delay.

On the other hand, a default cleanup timeout of 0.0 would mean that the child is killed prematurely, and the user experiences incorrect/incomplete cleanup behavior when hitting Ctrl+C. That's the situation Python 3 users are in today.

But what about a default cleanup delay of 1.0 second? If the child responds to SIGINT then the user experiences no extra delay. But for those (very few) children that don't actually respond to SIGINT, the consequences are relatively minor. The user would merely have to do one of the following:

1. Hit Ctrl+C again, OR
2. Do nothing... for 1 second.

The idea with this PR is to help Python programmers write code that "just works" in the default case. After all, the run() function is a convenience API. It's meant to help avoid boilerplate and spare the user from the details of the lower-level Popen API, if they don't want to think about those details. But right now, Python does the wrong thing by default, which is very un-Pythonic, if you ask me. :-P

(BTW, in Python 2, the behavior wasn't perfect, but it happened to be better-behaved in this one instance.)

You cannot assume that a child will get SIGINT, this is true only for a foreground process in the shell.

Question: After thinking about your comment some more, I'm now wondering: What do you think about explicitly sending SIGINT to the child, assuming we're handling a KeyboardInterrupt? (For other exceptions, we would send SIGTERM, as you suggested below.)

We should TERM (or, worse, KILL) it, not INTerrupt it.

Firstly, it is reasonable to assume the child already got the INT. More importantly, a TERM better fits the longstanding semantic conventions of signal use here.

While it's true that a SIGINT could be generated by some circumstance other than a terminal driver catching Ctrl-C and signaling the whole foreground process group (not individual PIDs), in practice that would be an exceedingly rare violation of longstanding convention. POSIX refers to SIGINT as the "terminal interrupt signal", for example. Indeed, the fact that python by default turns a SIGINT into a KeyboardInterrupt tells you something about its expected use.

I don't see benefit in second-guessing system behavior when an INT rolls in. If we're going to terminate the child, TERM the child — and please let devs like me suppress that behavior entirely. :)

Command lines tools are used both interactively and run by other programs. They are expected to handle both SIGINT and SIGTERM, and typically both are handled in the same way.

We can assume that a child process is handling both signals. If you run a program that needs special treatment, use Popen directly instead of the high level easy to use call() or run().

They are expected to handle both SIGINT and SIGTERM, and typically both are handled in the same way.

I completely agree that they should. But as Python programmers, it would be particularly odd for us to assume that they do. After all, tools written in Python itself will respond differently for SIGINT and SIGTERM -- unless the author was kind enough to register a handler for SIGTERM. Most authors do not bother.

For example, on a whim, I just checked a few popular utilities that are implemented in Python. Frankly, these results shocked me:

• httpie handlesSIGINT gracefully, but not SIGTERM. (I expected better from a project project that has 32,518 stars on github!)
• Another example seems to be the boto suite of AWS utilities. (I didn't verify via experiment, I can find no mention of SIGTERM in it's source code.)
• Holy crap, you guys aren't gonna believe this: Even pip doesn't handle SIGTERM gracefully. Apparently, if you send it SIGTERM, it does no cleanup at all -- you just end up with a partially installed python package (garbage, in other words). I'm legitimately surprised!

Again, I completely agree regarding what the proper POSIX conventions ought to be. But now we have to decide: we can either be POSIX purists, or be considerate to the typical user. I can see arguments in favor of either, but we should acknowledge the trade-off we're making.

The subprocess module is not the place to fix bad signal handling in other programs.

Maybe send a PR to httpie/pip?

+1 for making a PR to those projects. But of course there will always be others with the same issue, and we can't fix all of them.

I'm just wondering if we have an opportunity to apply the Zen of Python maxim: "practicality beats purity". I'm honestly not sure. But doing the most "friendly" thing (if we can figure out what that is) seems spiritually compatible with a convenience API like run(), even if it's less pure -- or maybe even a little ugly.

We cannot fix the issue in subprocess. For example we can have a correct program that block SIGINT when running without a controlling terminal. The only way to terminate the program is SIGTERM. We must support such program by sending SIGTERM. If another program misbehaves by not handling SIGTERM, the program author should fix the program.

I think we need much simple solution.

except:
    if kill_timeout is not None:
        process.terminate()
        try:
            process.wait(timeout=kill_timoeut)
         except:
             pass
    process.kill()
    process.wait()
    raise

Yes. Something like kill_timeout=None is the key here, letting the developer suppress the (in my view misguided) SIGKILL. That lets someone with my crusty POSIX sensibilities use an easy subprocess abstraction, and backport it to earlier python.

Regardless of the default timeout value, I do like this proposal to explicitly terminate() the child, rather than merely hoping it will just exit gracefully on its own after we close its stdin/stdout.

One open question is whether or not it is bad to send a SIGTERM to a process that has (probably) already received a SIGINT.

For example, if the child is a Python program, then the default behavior of SIGTERM is to end the process without any cleanup. If it has already received a SIGINT, then the process will likely end during cleanup.

From the perspective of the child process, a TERM is at worst equivalent to a KILL, and possibly much more polite, since it can be caught. So, if we are committed to killing the child process, I'd encourage a TERM first.

(It's tempting to full-on generalize this and allow something like timeout_handler=some_func_accepting_the_process_as_arg or err_kill=( (0.5, SIGTERM), (0.5, SIGQUIT), (1.0, SIGKILL) ) Those args are just illustrative, but you get the idea.)

I'd encourage a TERM first.

👍

It's tempting to full-on generalize this

Agree with the sentiment. I also completely agree with what you implied by using the word "tempting" (instead of, say, "justified" 😄 ). The run() API is meant for easy, general usage, with reasonable default behavior. If a programmer cares about nuanced error-handling, she should just use the Popen API.

kill_timeout may be more clear, the time we give child process to terminate before we kill it.

I am ambivalent regarding the parameter name.

Too complicated.

If cleanup timeout is not specified, there should be no cleanup timeout.

If cleanup timeout is not specified, there should be no cleanup timeout.

I assume you chose that phrasing to be pointedly simple, but, unfortunately, I think your phrasing is not as simple as it appears. In the subprocess API and docs, "no timeout" (i.e. timeout=None) actually means "timeout=infinity". That's probably not what you mean.

I think you meant to propose: "If cleanup timeout is not specified, there should be a cleanup timeout of 0.0".

@stuarteberg I agree that "no cleanup timeout" is not clear enough, but it is not the same as cleanup timeout of 0 seconds.

How about:

If kill_timeout is not specified, the child process will be killed immediately when the timeout expire. If specified, the child process will be terminated gracefully using SIGTERM. If the process did not terminate within kill_timeout seconds, it will be killed.

This should be modified if graceful termination should be the default behavior. I'm not sure what should be the default, but this can be left for another PR.