Windows Process Termination #39
Conversation
|
@Juvenal1228 : looking good! where you able to isolate a test that could consistently reproduce the issue? |
|
Yep, this looks good to me, but it would be good if we could add something which tests this behaviour, even if I'm not running the tests on windows by default. I don't suppose it would be too hard to write a test program which emits some text on a Ctrl-C but not a forceful termination, and then add a (windows-only?) integration test to ensure this code is getting called? |
|
I'd really like to merge this, but I'm reluctant to do so without some tests. @pombredanne, you've been brilliant with Windows stuff. Any chance you could help out here? |
|
(P.S. looks like the branch will need a rebase, too) |
|
@nickstenning not sure I am nor like to be brilliant on windows stuffs, but yes, I will take a stab at it. and without tests, no way you merge this in ! |
This is a large rework of the core of Honcho, namely the ProcessManager class, which in this commit is renamed to Manager. The motivations for this set of changes are: 1. In general, test coverage on Honcho is pretty poor, which makes making changes to the core risky. If the core of Honcho were beautifully designed and bug-free, this wouldn't be an issue, but... 2. ...there are a number of nasty bugs in Honcho which relate either to concurrency issues (#61) or inappropriate handling of child processes (#70, #39, #32). At least some of these bugs will need some quite substantial reworking of the core to fix. In particular, most of the test coverage comes from some rather nasty integration tests. Making any changes to Honcho's core require updating almost all the integration tests, and I want to get rid of them, but I'm not comfortable getting rid of them until we have sensible unit test coverage. So. This commit pulls apart `honcho.process` into two modules, `honcho.process` and `honcho.manager`, each with a start at some decent unit tests. `honcho.process` contains the Process class, which now handles spinning up a child process and forwarding lifecycle events and output from that process to a queue. `honcho.manager` contains the Manager class, which has an almost identical external API, but dramatically reworked internals. In particular, its hardcoded dependency on Process and Printer has been broken so that it can be more effectively tested. The division of responsibilities between Printer and Manager should now be clearer. The test coverage on Manager is still not what it should be, due at least partly to the difficulty of testing such a fundamentally parallel object. There is, however, the start of a reusable test harness in test_manager.py, which should make life easier for a range of tests. Lastly, most communication with the outside world (retrieval of the current time, os.kill, etc.) from Process or Manager is now mediated through `honcho.environ.Env` to make it easier to replace these functions with fakes when testing. This commit should fix #32, #59, #61 (and #62), #66, and #76.
On windows, process termination is always forceful. There is no chance for processes to do any cleanup.
This could be solved by using some ctypes kernel32 calls to send 'ctrl-c' to child processes, and then fall back on killing processes that still exist.
The code in this PR works both when running from the console, and when running honcho as a windows service.
If honcho is running in the console, ctrl-c events will be sent to all child processes already, so all that has to be done is give them time to do a graceful shutdown before killing them.
If honcho runs as a windows service, it first needs to connect to the child process consoles and raise the ctrl-c event, and then wait.
Unfortunately in windows when you kill a process its child processes are not also killed. So if any process honcho launches is forcefully killed and also launched its own children, they will become orphaned. The way around this is to use mozprocess (suggested by someone else previously) which keeps track of child processes and correctly kills them all. I have not implemented this, but it should be fairly simple.