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

[Adrian Buehlmann]

On 2012-06-19 11:58, Mads Kiilerich wrote:
> 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.

Ok. Thanks for this open statement.

> 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.

I can assure you that the execution time is totally unrelated to the
process killing.

That test simply takes that long on Windows. You can see that by running
it with --debug.

Ok folks. I'm now going to fork mercurial for the testsuite so I can run
those tests here.

I currently have no better idea how to kill processes. If it's not yet
ready for you, fine by me. If I find something better while playing with
my fork, I'll let you know.