[PATCH 1 of 2] win32: spawndetached returns pid of detached process and not of cmd.exe
Simon Heimberg
simohe at besonet.ch
Mon Feb 10 11:02:36 CST 2014
Am 10.02.2014 16:57, schrieb Mads Kiilerich:
> On 02/10/2014 03:37 PM, Simon Heimberg wrote:
>> # HG changeset patch
>> # User Simon Heimberg <simohe at besonet.ch>
>> # Date 1391866507 -3600
>> # Sat Feb 08 14:35:07 2014 +0100
>> # Node ID 1277051454b64dfd5b34d6fd8588f448be2ff47f
>> # Parent 12df27e151f1d72a180ab1e40222a9d88110e096
>> win32: spawndetached returns pid of detached process and not of cmd.exe
>>
>> win32.spawndetached starts the detached process by `cmd.exe` (or
>> COMSPEC). The
>> pid it returned was the one of cmd.exe and not the one of the
>> detached process.
>> When this pid is used to kill the process, the detached process is
>> not killed,
>> but only cmd.exe.
>>
>> With this patch the pid of the detached process is written to the pid
>> file.
>> Killing the process works as expected.
>>
>> The pid is only evaluated on writing the pid file. It is unnecessary
>> to search
>> the pid when it is not needed. And more important, it probably does
>> not yet
>> exist right after the cmd.exe process was started. When the pid is
>> written to
>> the file, waiting for the start of the detached process has already
>> happened.
>> Use this functionality instead of writing a 2nd wait function.
>>
>> Many tests on windows will not fail anymore, all those with the first
>> failing
>> line "abort: child process failed to start". (The processes still
>> hanging
>> around from previous test runs have to be killed first. They still
>> block a
>> tcp port.)
>> A good test for the functionality of this patch is test-treediscovery.t,
>> because it starts and kills `hg serve -d` several times.
>
> Assuming the analysis is correct and it actually works: Very nice! The
> windows process ids always seemed unreliable and unusable to me (and
> Patrick, IIRC). It sounds like it was very non-trivial but you nailed it.
>
> With PID writing working reliable on windows, the main showstopper for
> _really_ supporting daemon mode on windows is to have some reliable
> 'kill' command. I think it would make sense to have a cross platform
> 'hg serve --pid-file X --kill' option. That could also be used for a
> cross platform test: launch a daemon on $HGPORT, kill it, and verify
> that another one can be started on the same port.
Is this not is what test-treediscovery.t does? It starts `hg serve -d
--pid-file hg.pid`, does some test, kills it, starts it again at the
same port, ... It uses tests/killdaemons.py for killing, which seems to
work reliable also on windows (as long as the right pid is provided).
I would recommend to the user to use the kill command that the system
provides. (`kill PID` on linux, `taskkill /pid PID` or the task manager
on windows, ...) Or did I misunderstand the use of `hg serve --kill
--pid-file X`?
>
> (I agree that the ps test can be problematic. ps comes in completely
> different variants and for instance OS X do not have --no-heading.)
>
> /Mads
>
>> diff -r 12df27e151f1 -r 1277051454b6 mercurial/win32.py
>> --- a/mercurial/win32.py Sat Feb 08 17:48:57 2014 +0100
>> +++ b/mercurial/win32.py Sat Feb 08 14:35:07 2014 +0100
>> @@ -119,6 +119,27 @@
>> _STD_ERROR_HANDLE = _DWORD(-12).value
>> +# CreateToolhelp32Snapshot, Process32First, Process32Next
>> +_TH32CS_SNAPPROCESS = 0x00000002
>> +_MAX_PATH = 260
>> +
>> +class _tagPROCESSENTRY32(ctypes.Structure):
>> + _fields_ = [('dwsize', _DWORD),
>> + ('cntUsage', _DWORD),
>> + ('th32ProcessID', _DWORD),
>> + ('th32DefaultHeapID', ctypes.c_void_p),
>> + ('th32ModuleID', _DWORD),
>> + ('cntThreads', _DWORD),
>> + ('th32ParentProcessID', _DWORD),
>> + ('pcPriClassBase', _LONG),
>> + ('dwFlags', _DWORD),
>> + ('szExeFile', ctypes.c_char * _MAX_PATH)]
>> +
>> + def __init__(self):
>> + super(_tagPROCESSENTRY32, self).__init__()
>> + self.dwsize = ctypes.sizeof(self)
>> +
>> +
>> # types of parameters of C functions used (required by pypy)
>> _kernel32.CreateFileA.argtypes = [_LPCSTR, _DWORD, _DWORD,
>> ctypes.c_void_p,
>> @@ -186,6 +207,15 @@
>> _user32.EnumWindows.argtypes = [_WNDENUMPROC, _LPARAM]
>> _user32.EnumWindows.restype = _BOOL
>> +_kernel32.CreateToolhelp32Snapshot.argtypes = [_DWORD, _DWORD]
>> +_kernel32.CreateToolhelp32Snapshot.restype = _BOOL
>> +
>> +_kernel32.Process32First.argtypes = [_HANDLE, ctypes.c_void_p]
>> +_kernel32.Process32First.restype = _BOOL
>> +
>> +_kernel32.Process32Next.argtypes = [_HANDLE, ctypes.c_void_p]
>> +_kernel32.Process32Next.restype = _BOOL
>> +
>> def _raiseoserror(name):
>> err = ctypes.WinError()
>> raise OSError(err.errno, '%s: %s' % (name, err.strerror))
>> @@ -309,6 +339,50 @@
>> width = csbi.srWindow.Right - csbi.srWindow.Left
>> return width
>> +def _1stchild(pid):
>> + '''return the 1st found child of the given pid
>> +
>> + None is returned when no child is found'''
>> + pe = _tagPROCESSENTRY32()
>> +
>> + # create handle to list all processes
>> + ph = _kernel32.CreateToolhelp32Snapshot(_TH32CS_SNAPPROCESS, 0)
>> + if ph == _INVALID_HANDLE_VALUE:
>> + raise ctypes.WinError
>> + try:
>> + r = _kernel32.Process32First(ph, ctypes.byref(pe))
>> + # loop over all processes
>> + while r:
>> + if pe.th32ParentProcessID == pid:
>> + # return first child found
>> + return pe.th32ProcessID
>> + r = _kernel32.Process32Next(ph, ctypes.byref(pe))
>> + finally:
>> + _kernel32.CloseHandle(ph)
>> + if _kernel32.GetLastError() != _ERROR_NO_MORE_FILES:
>> + raise ctypes.WinError
>> + return None # no child found
>> +
>> +class _tochildpid(int): # pid is _DWORD, which always matches in an int
>> + '''helper for spawndetached, returns the child pid on conversion
>> to string
>> +
>> + Does not resolve the child pid immediately because the child may
>> not yet be
>> + started.
>> + '''
>> + def childpid(self):
>> + 'returns the child pid of the first found child of the
>> process with this pid'
>> + return _1stchild(self)
>> + def __str__(self):
>> + # run when the pid is written to the file
>> + ppid = self.childpid()
>> + if ppid is None:
>> + # race, child has exited since check
>> + # fall back to this pid. Its process will also have
>> disappered,
>> + # raising the same error type later as when the child
>> pid would
>> + # be returned.
>> + return " %d" % self
>> + return str(ppid)
>> +
>> def spawndetached(args):
>> # No standard library function really spawns a fully detached
>> # process under win32 because they allocate pipes or other objects
>> @@ -339,7 +413,8 @@
>> if not res:
>> raise ctypes.WinError
>> - return pi.dwProcessId
>> + # _tochildpid because the process is the child of COMSPEC
>> + return _tochildpid(pi.dwProcessId)
>> def unlink(f):
>> '''try to implement POSIX' unlink semantics on Windows'''
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
More information about the Mercurial-devel
mailing list