Merge pull request #1212 from meejah/3921.exit-on-stdin-close

exit on stdin close

.github/workflows/ci.yml

@@ -63,7 +63,7 @@ jobs:
             python-version: "pypy-3.7"
           - os: ubuntu-latest
             python-version: "pypy-3.8"
-    
+
     steps:
       # See https://github.com/actions/checkout. A fetch-depth of 0
       # fetches all tags and branches.

newsfragments/3921.feature

@@ -0,0 +1,5 @@
+`tahoe run ...` will now exit when its stdin is closed.
+
+This facilitates subprocess management, specifically cleanup.
+When a parent process is running tahoe and exits without time to do "proper" cleanup at least the stdin descriptor will be closed.
+Subsequently "tahoe run" notices this and exits.

src/allmydata/scripts/tahoe_run.py

@@ -21,7 +21,11 @@ from twisted.scripts import twistd
 from twisted.python import usage
 from twisted.python.filepath import FilePath
 from twisted.python.reflect import namedAny
-from twisted.internet.defer import maybeDeferred
+from twisted.python.failure import Failure
+from twisted.internet.defer import maybeDeferred, Deferred
+from twisted.internet.protocol import Protocol
+from twisted.internet.stdio import StandardIO
+from twisted.internet.error import ReactorNotRunning
 from twisted.application.service import Service
 
 from allmydata.scripts.default_nodedir import _default_nodedir
@@ -155,6 +159,8 @@ class DaemonizeTheRealService(Service, HookMixin):
 
     def startService(self):
 
+        from twisted.internet import reactor
+
         def start():
             node_to_instance = {
                 u"client": lambda: maybeDeferred(namedAny("allmydata.client.create_client"), self.basedir),
@@ -194,12 +200,14 @@ class DaemonizeTheRealService(Service, HookMixin):
 
             def created(srv):
                 srv.setServiceParent(self.parent)
+                # exiting on stdin-closed facilitates cleanup when run
+                # as a subprocess
+                on_stdin_close(reactor, reactor.stop)
             d.addCallback(created)
             d.addErrback(handle_config_error)
             d.addBoth(self._call_hook, 'running')
             return d
 
-        from twisted.internet import reactor
         reactor.callWhenRunning(start)
 
 
@@ -213,6 +221,46 @@ class DaemonizeTahoeNodePlugin(object):
         return DaemonizeTheRealService(self.nodetype, self.basedir, so)
 
 
+def on_stdin_close(reactor, fn):
+    """
+    Arrange for the function `fn` to run when our stdin closes
+    """
+    when_closed_d = Deferred()
+
+    class WhenClosed(Protocol):
+        """
+        Notify a Deferred when our connection is lost .. as this is passed
+        to twisted's StandardIO class, it is used to detect our parent
+        going away.
+        """
+
+        def connectionLost(self, reason):
+            when_closed_d.callback(None)
+
+    def on_close(arg):
+        try:
+            fn()
+        except ReactorNotRunning:
+            pass
+        except Exception:
+            # for our "exit" use-case failures will _mostly_ just be
+            # ReactorNotRunning (because we're already shutting down
+            # when our stdin closes) but no matter what "bad thing"
+            # happens we just want to ignore it .. although other
+            # errors might be interesting so we'll log those
+            print(Failure())
+        return arg
+
+    when_closed_d.addBoth(on_close)
+    # we don't need to do anything with this instance because it gets
+    # hooked into the reactor and thus remembered .. but we return it
+    # for Windows testing purposes.
+    return StandardIO(
+        proto=WhenClosed(),
+        reactor=reactor,
+    )
+
+
 def run(reactor, config, runApp=twistd.runApp):
     """
     Runs a Tahoe-LAFS node in the foreground.

src/allmydata/test/test_runner.py

@@ -47,6 +47,9 @@ from twisted.internet.defer import (
     inlineCallbacks,
     DeferredList,
 )
+from twisted.internet.testing import (
+    MemoryReactorClock,
+)
 from twisted.python.filepath import FilePath
 from allmydata.util import fileutil, pollmixin
 from allmydata.util.encodingutil import unicode_to_argv
@@ -60,6 +63,9 @@ import allmydata
 from allmydata.scripts.runner import (
     parse_options,
 )
+from allmydata.scripts.tahoe_run import (
+    on_stdin_close,
+)
 
 from .common import (
     PIPE,
@@ -624,6 +630,64 @@ class RunNode(common_util.SignalMixin, unittest.TestCase, pollmixin.PollMixin):
         yield client_running
 
 
+def _simulate_windows_stdin_close(stdio):
+    """
+    on Unix we can just close all the readers, correctly "simulating"
+    a stdin close .. of course, Windows has to be difficult
+    """
+    stdio.writeConnectionLost()
+    stdio.readConnectionLost()
+
+
+class OnStdinCloseTests(SyncTestCase):
+    """
+    Tests for on_stdin_close
+    """
+
+    def test_close_called(self):
+        """
+        our on-close method is called when stdin closes
+        """
+        reactor = MemoryReactorClock()
+        called = []
+
+        def onclose():
+            called.append(True)
+        transport = on_stdin_close(reactor, onclose)
+        self.assertEqual(called, [])
+
+        if platform.isWindows():
+            _simulate_windows_stdin_close(transport)
+        else:
+            for reader in reactor.getReaders():
+                reader.loseConnection()
+            reactor.advance(1)  # ProcessReader does a callLater(0, ..)
+
+        self.assertEqual(called, [True])
+
+    def test_exception_ignored(self):
+        """
+        An exception from our on-close function is discarded.
+        """
+        reactor = MemoryReactorClock()
+        called = []
+
+        def onclose():
+            called.append(True)
+            raise RuntimeError("unexpected error")
+        transport = on_stdin_close(reactor, onclose)
+        self.assertEqual(called, [])
+
+        if platform.isWindows():
+            _simulate_windows_stdin_close(transport)
+        else:
+            for reader in reactor.getReaders():
+                reader.loseConnection()
+            reactor.advance(1)  # ProcessReader does a callLater(0, ..)
+
+        self.assertEqual(called, [True])
+
+
 class PidFileLocking(SyncTestCase):
     """
     Direct tests for allmydata.util.pid functions

tox.ini

@@ -86,7 +86,6 @@ commands =
          coverage: python -b -m coverage run -m twisted.trial {env:TAHOE_LAFS_TRIAL_ARGS:--rterrors --reporter=timing} {posargs:{env:TEST_SUITE}}
          coverage: coverage combine
          coverage: coverage xml
-         coverage: coverage report
 
 [testenv:integration]
 basepython = python3
@@ -99,7 +98,6 @@ commands =
          # NOTE: 'run with "py.test --keep-tempdir -s -v integration/" to debug failures'
          py.test --timeout=1800 --coverage -s -v {posargs:integration}
          coverage combine
-         coverage report
 
 
 [testenv:codechecks]