[PATCH 1 of 2] win32: spawndetached returns pid of detached process and not of cmd.exe

Mads Kiilerich mads at kiilerich.com
Mon Feb 10 09:57:34 CST 2014


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.

(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