From e8dcf27e474aade3d7b14b9592383417ce841288 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Wed, 23 May 2018 20:04:56 -0400 Subject: [PATCH 1/2] Fix subprocess.close() to let its processes die gracefully Fixes #128. --- tests/test_process.py | 26 ++++++++++++++++++++++++++ uvloop/handles/process.pxd | 2 ++ uvloop/handles/process.pyx | 17 ++++++++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/test_process.py b/tests/test_process.py index 67ee3da0..16aca61d 100644 --- a/tests/test_process.py +++ b/tests/test_process.py @@ -580,6 +580,32 @@ def cancel_make_transport(): 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): diff --git a/uvloop/handles/process.pxd b/uvloop/handles/process.pxd index dcb144e3..f94a1cc8 100644 --- a/uvloop/handles/process.pxd +++ b/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, diff --git a/uvloop/handles/process.pyx b/uvloop/handles/process.pyx index 9b76177c..50f39c8f 100644 --- a/uvloop/handles/process.pyx +++ b/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, @@ -303,6 +304,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(self._handle, signum) if err < 0: raise convert_error(err) @@ -532,6 +535,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: @@ -628,7 +636,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 From 6ec2432e11e0d6279ec7a3099048f107d06816dc Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Thu, 24 May 2018 20:36:20 -0400 Subject: [PATCH 2/2] Fix tests / debug mode --- tests/test_process.py | 4 ++++ uvloop/handles/handle.pyx | 2 +- uvloop/handles/process.pyx | 19 ++++++++++++++++++- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/test_process.py b/tests/test_process.py index 16aca61d..41c2e3bc 100644 --- a/tests/test_process.py +++ b/tests/test_process.py @@ -1,5 +1,6 @@ import asyncio import contextlib +import gc import os import pathlib import signal @@ -574,6 +575,9 @@ 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(): diff --git a/uvloop/handles/handle.pyx b/uvloop/handles/handle.pyx index 2c99cacb..1e1d1d1a 100644 --- a/uvloop/handles/handle.pyx +++ b/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( diff --git a/uvloop/handles/process.pyx b/uvloop/handles/process.pyx index 50f39c8f..2f0768c4 100644 --- a/uvloop/handles/process.pyx +++ b/uvloop/handles/process.pyx @@ -183,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 @@ -192,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 @@ -554,6 +556,21 @@ cdef class UVProcessTransport(UVProcess): 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,