mitogen: Fix subprocess ResourceWarning

Python 3.x emits `ResourceWarning`s if certains resources aren't correctly
closed. Due to the way Mitogen has been terminating child processes this has
been occurring.

```
test_dev_tty_open_succeeds
(create_child_test.TtyCreateChildTest.test_dev_tty_open_succeeds) ...
/opt/hostedtoolcache/Python/3.12.5/x64/lib/python3.12/subprocess.py:1127:
ResourceWarning: subprocess 3313 is still running
  _warn("subprocess %s is still running" % self.pid,
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
```

During garbage collection subprocess.Popen() objects emit
ResourceWarning("subprocess 123 is still running")
if proc.returncode hasn't been set. Typically calling proc.wait() does so,
once the sub-process has exited. Calling os.waitpid(proc.pid, 0) also waits
for the sub-process to exit, but it doesn't update proc.returncode, so the
ResourceWarning is still emitted.

This change exposes `subprocess.Popen` methods on
`mitogen.parent.PopenProcess`, so that the returncode can be set.

See https://gist.github.com/moreati/b8d157ff82cb15234bece4033accc5e5
pull/1119/head
Alex Willmer 2 months ago
parent 7238403392
commit 598de81143

@ -2542,7 +2542,7 @@ class Reaper(object):
# because it is setuid, so this is best-effort only. # because it is setuid, so this is best-effort only.
LOG.debug('%r: sending %s', self.proc, SIGNAL_BY_NUM[signum]) LOG.debug('%r: sending %s', self.proc, SIGNAL_BY_NUM[signum])
try: try:
os.kill(self.proc.pid, signum) self.proc.send_signal(signum)
except OSError: except OSError:
e = sys.exc_info()[1] e = sys.exc_info()[1]
if e.args[0] != errno.EPERM: if e.args[0] != errno.EPERM:
@ -2662,6 +2662,17 @@ class Process(object):
""" """
raise NotImplementedError() raise NotImplementedError()
def send_signal(self, sig):
os.kill(self.pid, sig)
def terminate(self):
"Ask the process to gracefully shutdown."
self.send_signal(signal.SIGTERM)
def kill(self):
"Ask the operating system to forcefully destroy the process."
self.send_signal(signal.SIGKILL)
class PopenProcess(Process): class PopenProcess(Process):
""" """
@ -2678,6 +2689,9 @@ class PopenProcess(Process):
def poll(self): def poll(self):
return self.proc.poll() return self.proc.poll()
def send_signal(self, sig):
self.proc.send_signal(sig)
class ModuleForwarder(object): class ModuleForwarder(object):
""" """

@ -55,7 +55,9 @@ def do_detach(econtext):
class DetachReapTest(testlib.RouterMixin, testlib.TestCase): class DetachReapTest(testlib.RouterMixin, testlib.TestCase):
def test_subprocess_preserved_on_shutdown(self): def test_subprocess_preserved_on_shutdown(self):
c1 = self.router.local() c1 = self.router.local()
c1_stream = self.router.stream_by_id(c1.context_id)
pid = c1.call(os.getpid) pid = c1.call(os.getpid)
self.assertEqual(pid, c1_stream.conn.proc.pid)
l = mitogen.core.Latch() l = mitogen.core.Latch()
mitogen.core.listen(c1, 'disconnect', l.put) mitogen.core.listen(c1, 'disconnect', l.put)
@ -65,8 +67,8 @@ class DetachReapTest(testlib.RouterMixin, testlib.TestCase):
self.broker.shutdown() self.broker.shutdown()
self.broker.join() self.broker.join()
os.kill(pid, 0) # succeeds if process still alive self.assertIsNone(os.kill(pid, 0)) # succeeds if process still alive
# now clean up # now clean up
os.kill(pid, signal.SIGTERM) c1_stream.conn.proc.terminate()
os.waitpid(pid, 0) c1_stream.conn.proc.proc.wait()

@ -76,6 +76,7 @@ def close_proc(proc):
proc.stdout.close() proc.stdout.close()
if proc.stderr: if proc.stderr:
proc.stderr.close() proc.stderr.close()
proc.proc.wait()
def wait_read(fp, n): def wait_read(fp, n):

@ -10,8 +10,7 @@ import mitogen.parent
class ReaperTest(testlib.TestCase): class ReaperTest(testlib.TestCase):
@mock.patch('os.kill') def test_calc_delay(self):
def test_calc_delay(self, kill):
broker = mock.Mock() broker = mock.Mock()
proc = mock.Mock() proc = mock.Mock()
proc.poll.return_value = None proc.poll.return_value = None
@ -24,8 +23,7 @@ class ReaperTest(testlib.TestCase):
self.assertEqual(752, int(1000 * reaper._calc_delay(5))) self.assertEqual(752, int(1000 * reaper._calc_delay(5)))
self.assertEqual(1294, int(1000 * reaper._calc_delay(6))) self.assertEqual(1294, int(1000 * reaper._calc_delay(6)))
@mock.patch('os.kill') def test_reap_calls(self):
def test_reap_calls(self, kill):
broker = mock.Mock() broker = mock.Mock()
proc = mock.Mock() proc = mock.Mock()
proc.poll.return_value = None proc.poll.return_value = None
@ -33,20 +31,20 @@ class ReaperTest(testlib.TestCase):
reaper = mitogen.parent.Reaper(broker, proc, True, True) reaper = mitogen.parent.Reaper(broker, proc, True, True)
reaper.reap() reaper.reap()
self.assertEqual(0, kill.call_count) self.assertEqual(0, proc.send_signal.call_count)
reaper.reap() reaper.reap()
self.assertEqual(1, kill.call_count) self.assertEqual(1, proc.send_signal.call_count)
reaper.reap() reaper.reap()
reaper.reap() reaper.reap()
reaper.reap() reaper.reap()
self.assertEqual(1, kill.call_count) self.assertEqual(1, proc.send_signal.call_count)
reaper.reap() reaper.reap()
self.assertEqual(2, kill.call_count) self.assertEqual(2, proc.send_signal.call_count)
self.assertEqual(kill.mock_calls, [ self.assertEqual(proc.send_signal.mock_calls, [
mock.call(proc.pid, signal.SIGTERM), mock.call(signal.SIGTERM),
mock.call(proc.pid, signal.SIGKILL), mock.call(signal.SIGKILL),
]) ])

Loading…
Cancel
Save