Cleanup process finalization logic.

A follow up to fixes for #128.

Author: 1st1

tests/test_process.py

@@ -557,6 +557,10 @@ def cancel_make_transport():
             except asyncio.CancelledError:
                 pass
 
+            # Give the process handler some time to close itself
+            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():
@@ -575,6 +579,7 @@ def cancel_make_transport():
             except asyncio.CancelledError:
                 pass
 
+            # Give the process handler some time to close itself
             yield from asyncio.sleep(0.3, loop=self.loop)
             gc.collect()
 

uvloop/handles/process.pxd

@@ -20,8 +20,6 @@ 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,7 +10,6 @@ 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,
@@ -115,6 +114,14 @@ cdef class UVProcess(UVHandle):
         # after the process is finished.
         self._pid = (<uv.uv_process_t*>self._handle).pid
 
+        # Track the process handle (create a strong ref to it)
+        # to guarantee that __dealloc__ doesn't happen in an
+        # uncontrolled fashion.  We want to wait until the process
+        # exits and libuv calls __uvprocess_on_exit_callback,
+        # which will call `UVProcess._close()`, which will, in turn,
+        # untrack this handle.
+        self._loop._track_process(self)
+
         for fd in restore_inheritable:
             os_set_inheritable(fd, False)
 
@@ -133,7 +140,7 @@ cdef class UVProcess(UVHandle):
                 exc_name, exc_msg = errpipe_data.split(b':', 1)
                 exc_name = exc_name.decode()
                 exc_msg = exc_msg.decode()
-            except:
+            except Exception:
                 self._close()
                 raise subprocess_SubprocessError(
                     'Bad exception data from child: {!r}'.format(
@@ -306,8 +313,6 @@ 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)
@@ -323,6 +328,13 @@ cdef class UVProcess(UVHandle):
 
         self._close()
 
+    cdef _close(self):
+        try:
+            if self._loop is not None:
+                self._loop._untrack_process(self)
+        finally:
+            UVHandle._close(self)
+
 
 DEF _CALL_PIPE_DATA_RECEIVED = 0
 DEF _CALL_PIPE_CONNECTION_LOST = 1
@@ -360,6 +372,8 @@ cdef class UVProcessTransport(UVProcess):
                 waiter.set_result(self._returncode)
         self._exit_waiters.clear()
 
+        self._close()
+
     cdef _check_proc(self):
         if not self._is_alive() or self._returncode is not None:
             raise ProcessLookupError()
@@ -537,11 +551,6 @@ 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:
@@ -556,21 +565,6 @@ 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,

uvloop/loop.pxd

@@ -58,6 +58,7 @@ cdef class Loop:
         set _servers
 
         object _transports
+        set _processes
         dict _fd_to_reader_fileobj
         dict _fd_to_writer_fileobj
 
@@ -170,6 +171,9 @@ cdef class Loop:
     cdef _fileobj_to_fd(self, fileobj)
     cdef _ensure_fd_no_transport(self, fd)
 
+    cdef _track_process(self, UVProcess proc)
+    cdef _untrack_process(self, UVProcess proc)
+
     cdef _new_reader_future(self, sock)
     cdef _new_writer_future(self, sock)
     cdef _add_reader(self, fd, Handle handle)

uvloop/loop.pyx

@@ -101,6 +101,7 @@ cdef class Loop:
         self._stopping = 0
 
         self._transports = weakref_WeakValueDictionary()
+        self._processes = set()
 
         # Used to keep a reference (and hence keep the fileobj alive)
         # for as long as its registered by add_reader or add_writer.
@@ -613,6 +614,12 @@ cdef class Loop:
     cdef _track_transport(self, UVBaseTransport transport):
         self._transports[transport._fileno()] = transport
 
+    cdef _track_process(self, UVProcess proc):
+        self._processes.add(proc)
+
+    cdef _untrack_process(self, UVProcess proc):
+        self._processes.discard(proc)
+
     cdef _fileobj_to_fd(self, fileobj):
         """Return a file descriptor from a file object.