Fix subprocess.close() to let its processes die gracefully (#151)

* Fix subprocess.close() to let its processes die gracefully

Fixes #128.

Author: 1st1

tests/test_process.py

@@ -1,5 +1,6 @@
 import asyncio
 import contextlib
+import gc
 import os
 import pathlib
 import signal
@@ -574,12 +575,41 @@ def cancel_make_transport():
             except asyncio.CancelledError:
                 pass
 
+            yield from asyncio.sleep(0.3, loop=self.loop)
+            gc.collect()
+
         # ignore the log:
         # "Exception during subprocess creation, kill the subprocess"
         with tb.disable_logger():
             self.loop.run_until_complete(cancel_make_transport())
             tb.run_briefly(self.loop)
 
+    def test_close_gets_process_closed(self):
+
+        loop = self.loop
+
+        class Protocol(asyncio.SubprocessProtocol):
+
+            def __init__(self):
+                self.closed = loop.create_future()
+
+            def connection_lost(self, exc):
+                self.closed.set_result(1)
+
+        @asyncio.coroutine
+        def test_subprocess():
+            transport, protocol = yield from loop.subprocess_exec(
+                Protocol, *self.PROGRAM_BLOCKED)
+            pid = transport.get_pid()
+            transport.close()
+            self.assertIsNone(transport.get_returncode())
+            yield from protocol.closed
+            self.assertIsNotNone(transport.get_returncode())
+            with self.assertRaises(ProcessLookupError):
+                os.kill(pid, 0)
+
+        loop.run_until_complete(test_subprocess())
+
 
 class Test_UV_Process(_TestProcess, tb.UVTestCase):
 

uvloop/handles/handle.pyx

@@ -55,7 +55,7 @@ cdef class UVHandle:
                 '{} is open in __dealloc__ with loop set to NULL'
                 .format(self.__class__.__name__))
 
-        if self._closed == 1:
+        if self._closed:
             # So _handle is not NULL and self._closed == 1?
             raise RuntimeError(
                 '{}.__dealloc__: _handle is NULL, _closed == 1'.format(

uvloop/handles/process.pxd

@@ -20,6 +20,8 @@ cdef class UVProcess(UVHandle):
         char *uv_opt_file
         bytes __cwd
 
+        bint _kill_requested
+
     cdef _init(self, Loop loop, list args, dict env, cwd,
                start_new_session,
                _stdin, _stdout, _stderr, pass_fds,

uvloop/handles/process.pyx

@@ -10,6 +10,7 @@ cdef class UVProcess(UVHandle):
         self._fds_to_close = set()
         self._preexec_fn = None
         self._restore_signals = True
+        self._kill_requested = False
 
     cdef _init(self, Loop loop, list args, dict env,
                cwd, start_new_session,
@@ -182,7 +183,7 @@ cdef class UVProcess(UVHandle):
                 'UVProcess._close_after_spawn called after uv_spawn')
         self._fds_to_close.add(fd)
 
-    def __dealloc__(self):
+    cdef _dealloc_impl(self):
         if self.uv_opt_env is not NULL:
             PyMem_RawFree(self.uv_opt_env)
             self.uv_opt_env = NULL
@@ -191,6 +192,8 @@ cdef class UVProcess(UVHandle):
             PyMem_RawFree(self.uv_opt_args)
             self.uv_opt_args = NULL
 
+        UVHandle._dealloc_impl(self)
+
     cdef char** __to_cstring_array(self, list arr):
         cdef:
             int i
@@ -303,6 +306,8 @@ cdef class UVProcess(UVHandle):
     cdef _kill(self, int signum):
         cdef int err
         self._ensure_alive()
+        if signum in {uv.SIGKILL, uv.SIGTERM}:
+            self._kill_requested = True
         err = uv.uv_process_kill(<uv.uv_process_t*>self._handle, signum)
         if err < 0:
             raise convert_error(err)
@@ -532,6 +537,11 @@ cdef class UVProcessTransport(UVProcess):
             else:
                 self._pending_calls.append((_CALL_CONNECTION_LOST, None, None))
 
+    cdef _warn_unclosed(self):
+        if self._kill_requested:
+            return
+        super()._warn_unclosed()
+
     def __stdio_inited(self, waiter, stdio_fut):
         exc = stdio_fut.exception()
         if exc is not None:
@@ -546,6 +556,21 @@ cdef class UVProcessTransport(UVProcess):
                                   <method1_t>self._call_connection_made,
                                   self, waiter))
 
+    cdef _dealloc_impl(self):
+        cdef int fix_needed
+
+        if UVLOOP_DEBUG:
+            # Check when __dealloc__ will simply call uv.uv_close()
+            # directly, thus *skipping* incrementing the debug counter;
+            # we need to fix that.
+            fix_needed = not self._closed and self._inited
+
+        UVProcess._dealloc_impl(self)
+
+        if UVLOOP_DEBUG and fix_needed and self._kill_requested:
+            self._loop._debug_handles_closed.update([
+                self.__class__.__name__])
+
     @staticmethod
     cdef UVProcessTransport new(Loop loop, protocol, args, env,
                                 cwd, start_new_session,
@@ -628,7 +653,14 @@ cdef class UVProcessTransport(UVProcess):
         if self._stderr is not None:
             self._stderr.close()
 
-        self._close()
+        if self._returncode is not None:
+            # The process is dead, just close the UV handle.
+            #
+            # (If "self._returncode is None", the process should have been
+            # killed already and we're just waiting for a SIGCHLD; after
+            # which the transport will be GC'ed and the uvhandle will be
+            # closed in UVHandle.__dealloc__.)
+            self._close()
 
     def get_extra_info(self, name, default=None):
         return default