Merge branch 'master' into 3899.failed-server

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

src/allmydata/test/test_testing.py

@@ -9,18 +9,7 @@
 
 """
 Tests for the allmydata.testing helpers
-
-Ported to Python 3.
-
 """
-from __future__ import absolute_import
-from __future__ import division
-from __future__ import print_function
-from __future__ import unicode_literals
-
-from future.utils import PY2
-if PY2:
-    from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min  # noqa: F401
 
 from twisted.internet.defer import (
     inlineCallbacks,
@@ -56,10 +45,12 @@ from testtools.matchers import (
     IsInstance,
     MatchesStructure,
     AfterPreprocessing,
+    Contains,
 )
 from testtools.twistedsupport import (
     succeeded,
 )
+from twisted.web.http import GONE
 
 
 class FakeWebTest(SyncTestCase):
@@ -144,7 +135,8 @@ class FakeWebTest(SyncTestCase):
 
     def test_download_missing(self):
         """
-        Error if we download a capability that doesn't exist
+        The response to a request to download a capability that doesn't exist
+        is 410 (GONE).
         """
 
         http_client = create_tahoe_treq_client()
@@ -157,7 +149,11 @@ class FakeWebTest(SyncTestCase):
             resp,
             succeeded(
                 MatchesStructure(
-                    code=Equals(500)
+                    code=Equals(GONE),
+                    content=AfterPreprocessing(
+                        lambda m: m(),
+                        succeeded(Contains(b"No data for")),
+                    ),
                 )
             )
         )

src/allmydata/testing/web.py

@@ -6,20 +6,12 @@
 # This file is part of Tahoe-LAFS.
 #
 # See the docs/about.rst file for licensing information.
-"""Test-helpers for clients that use the WebUI.
-
-Ported to Python 3.
 
 """
-from __future__ import absolute_import
-from __future__ import division
-from __future__ import print_function
-from __future__ import unicode_literals
-
-from future.utils import PY2
-if PY2:
-    from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min  # noqa: F401
+Test-helpers for clients that use the WebUI.
+"""
 
+from __future__ import annotations
 
 import hashlib
 
@@ -54,6 +46,7 @@ import allmydata.uri
 from allmydata.util import (
     base32,
 )
+from ..util.dictutil import BytesKeyDict
 
 
 __all__ = (
@@ -147,7 +140,7 @@ class _FakeTahoeUriHandler(Resource, object):
 
     isLeaf = True
 
-    data = attr.ib(default=attr.Factory(dict))
+    data: BytesKeyDict = attr.ib(default=attr.Factory(BytesKeyDict))
     capability_generators = attr.ib(default=attr.Factory(dict))
 
     def _generate_capability(self, kind):
@@ -209,7 +202,7 @@ class _FakeTahoeUriHandler(Resource, object):
         capability = None
         for arg, value in uri.query:
             if arg == u"uri":
-                capability = value
+                capability = value.encode("utf-8")
         # it's legal to use the form "/uri/<capability>"
         if capability is None and request.postpath and request.postpath[0]:
             capability = request.postpath[0]
@@ -221,10 +214,9 @@ class _FakeTahoeUriHandler(Resource, object):
 
         # the user gave us a capability; if our Grid doesn't have any
         # data for it, that's an error.
-        capability = capability.encode('ascii')
         if capability not in self.data:
-            request.setResponseCode(http.BAD_REQUEST)
-            return u"No data for '{}'".format(capability.decode('ascii'))
+            request.setResponseCode(http.GONE)
+            return u"No data for '{}'".format(capability.decode('ascii')).encode("utf-8")
 
         return self.data[capability]
 

src/allmydata/util/dictutil.py

@@ -1,21 +1,6 @@
 """
 Tools to mess with dicts.
-
-Ported to Python 3.
 """
-from __future__ import absolute_import
-from __future__ import division
-from __future__ import print_function
-from __future__ import unicode_literals
-
-from future.utils import PY2
-if PY2:
-    # IMPORTANT: We deliberately don't import dict. The issue is that we're
-    # subclassing dict, so we'd end up exposing Python 3 dict APIs to lots of
-    # code that doesn't support it.
-    from builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, list, object, range, str, max, min  # noqa: F401
-from six import ensure_str
-
 
 class DictOfSets(dict):
     def add(self, key, value):
@@ -104,7 +89,7 @@ def _make_enforcing_override(K, method_name):
             raise TypeError("{} must be of type {}".format(
                 repr(key), self.KEY_TYPE))
         return getattr(dict, method_name)(self, key, *args, **kwargs)
-    f.__name__ = ensure_str(method_name)
+    f.__name__ = method_name
     setattr(K, method_name, f)
 
 for _method_name in ["__setitem__", "__getitem__", "setdefault", "get",
@@ -113,18 +98,13 @@ for _method_name in ["__setitem__", "__getitem__", "setdefault", "get",
 del _method_name
 
 
-if PY2:
-    # No need for enforcement, can use either bytes or unicode as keys and it's
-    # fine.
-    BytesKeyDict = UnicodeKeyDict = dict
-else:
-    class BytesKeyDict(_TypedKeyDict):
+class BytesKeyDict(_TypedKeyDict):
     """Keys should be bytes."""
 
     KEY_TYPE = bytes
 
 
-    class UnicodeKeyDict(_TypedKeyDict):
+class UnicodeKeyDict(_TypedKeyDict):
     """Keys should be unicode strings."""
 
     KEY_TYPE = str

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]