[PATCH 1 of 2] run-tests: implement proper process killing for Windows

Mads Kiilerich mads at kiilerich.com
Tue Jun 19 04:58:52 CDT 2012


On 18/06/12 15:22, Adrian Buehlmann wrote:
> On 2012-06-18 14:48, Mads Kiilerich wrote:
>> On 18/06/12 02:39, Adrian Buehlmann wrote:
>>> On 2012-06-18 02:20, Mads Kiilerich wrote:
>>>> Adrian Buehlmann wrote, On 06/18/2012 12:35 AM:
>>>>> # HG changeset patch
>>>>> # User Adrian Buehlmann <adrian at cadifra.com>
>>>>> # Date 1339972395 -7200
>>>>> # Node ID 954a063d12edfe1d2a3d32df1a540c0ee5268156
>>>>> # Parent  191f0c6cc47c1cf85372f5e5943ca94aa14fcb65
>>>>> run-tests: implement proper process killing for Windows
>>>> This is allegedly a hard problem. It is great to have it solved!
>>> Not at all. I just tried to avoid looking into it so far. It's not
>>> really that much fun to have to jump through all these hoops here.
>>>
>>>>> diff --git a/tests/run-tests.py b/tests/run-tests.py
>>>>> --- a/tests/run-tests.py
>>>>> +++ b/tests/run-tests.py
>>>>> @@ -347,6 +347,25 @@
>>>>>         except OSError:
>>>>>             pass
>>>>>     
>>>>> +if os.name == 'nt':
>>>>> +    import ctypes
>>>>> +    def killprocess(pid):
>>>>> +        vlog('# Killing daemon process %d' % pid)
>>>>> +        PROCESS_TERMINATE = 1
>>>>> +        k = ctypes.windll.kernel32
>>>>> +        handle = k.OpenProcess(PROCESS_TERMINATE, False, pid)
>>>>> +        k.TerminateProcess(handle, -1)
>>>>> +        k.CloseHandle(handle)
>>>> How is this different from os.kill ... or msys kill? Why can't os.kill
>>>> be used? I assume you have done some research and found something that
>>>> would be helpful to have in the commit message for the next guy looking
>>>> at this.
>>> The difference is that os.kill is claimed to be implemented in Python
>>> 2.7, but I found it to be defunct. I haven't investigated why and in
>>> what ways exactly.
>>>
>>> I then copied the snippet above from a stackoverflow discussion and
>>> pasted it into run-tests like this. They said, it also works for Python
>>> 2.5, where os.kill isn't implemented (which makes sense). Note that
>>> ctypes first appeared in the Python standard lib with Python 2.5. And
>>> mercurial already depends on it (see win32.py).
>>>
>>> And I found it works pretty well, reliably kills the respective python
>>> process (verified on Windows 7). I also haven't investigated why.
>> Right; it is a killer argument that we need the ctypes implementation to
>> support Python 2.6. But it is strange that 2.7 os.kill doesn't work.
>>
>> Some tests use the shell 'kill' command, but IIRC it didn't work. (It
>> would be nice to know why.) The tests cold perhaps use killdaemons.py
>> instead, which suggests that the common kill implementation should live
>> there instead.
>>
>> (BTW: I get a vague recollection that the problems I have seen might
>> have been related to killing whole process trees.)
>>
>> In the bigger perspective we have 'hg serve --daemon' which works on
>> windows. On unix the daemon can easily be killed with the ubiquitous
>> kill command, but there is no convenient way to stop a serve daemon on
>> windows. That could be considered a bug that perhaps could be solved by
>> introducing something like 'hg serve --kill 117 118 119' or 'hg serve
>> --kill --pid-file daemon.pids'. It could thus make sense to let the code
>> live in 'oskill' in win32.py / util.py from the beginning.
> In other words: you don't want my patch?

It certainly is a valuable contribution that we all want, but I am not 
convinced it is ready yet, I am not convinced it actually solves the 
problem better than the obvious alternatives, and I am not sure it is a 
step in the right direction ... because I don't know what your goal is.

Patrick had problems with hanging processes, and I also saw enough 
problems to 'postpone' the issue without understanding it or finding a 
solution. It thus seems like it is a tricky area.

The test suite is quite stable on windows so it would be sad to 
destabilize it. I would like to see process killing working reliably in 
a bigger number of tests before taking the patch.


The handling of ESRCH OSError for ctypes seems like something that 
should be fixed.

The execution time you mentioned seems strange. It seems to be a problem 
that should be solved before enabling test-http-clone-r.t. We should 
convince ourselves that it isn't related to the process killing.

/Mads



More information about the Mercurial-devel mailing list