Merge pull request #1312 from exarkun/4044.simpler-webish-test

Make a TahoeLAFSSite test simpler and more reliable

Fixes: ticket:4044

src/allmydata/client.py

@@ -1,5 +1,5 @@
 """
-Ported to Python 3.
+Functionality related to operating a Tahoe-LAFS node (client _or_ server).
 """
 from __future__ import annotations
 
@@ -1029,14 +1029,14 @@ def _get_tempdir(self):
     def init_web(self, webport):
         self.log("init_web(webport=%s)", args=(webport,))
 
-        from allmydata.webish import WebishServer
+        from allmydata.webish import WebishServer, anonymous_tempfile_factory
         nodeurl_path = self.config.get_config_path("node.url")
         staticdir_config = self.config.get_config("node", "web.static", "public_html")
         staticdir = self.config.get_config_path(staticdir_config)
         ws = WebishServer(
             self,
             webport,
-            self._get_tempdir(),
+            anonymous_tempfile_factory(self._get_tempdir()),
             nodeurl_path,
             staticdir,
         )

src/allmydata/test/web/test_web.py

@@ -341,7 +341,7 @@ def setUp(self):
         self.ws = webish.WebishServer(
             self.s,
             "0",
-            tempdir=tempdir.path,
+            webish.anonymous_tempfile_factory(tempdir.path),
             staticdir=self.staticdir,
             clock=self.clock,
             now_fn=lambda:self.fakeTime,

src/allmydata/test/web/test_webish.py

@@ -1,23 +1,11 @@
 """
 Tests for ``allmydata.webish``.
-
-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
 
+import tempfile
 from uuid import (
     uuid4,
 )
-from errno import (
-    EACCES,
-)
 from io import (
     BytesIO,
 )
@@ -39,9 +27,6 @@
     HasLength,
 )
 
-from twisted.python.runtime import (
-    platform,
-)
 from twisted.python.filepath import (
     FilePath,
 )
@@ -59,6 +44,7 @@
 from ...webish import (
     TahoeLAFSRequest,
     TahoeLAFSSite,
+    anonymous_tempfile_factory,
 )
 
 
@@ -183,8 +169,14 @@ def _test_censoring(self, path, censored):
         :return: ``None`` if the logging looks good.
         """
         logPath = self.mktemp()
+        tempdir = self.mktemp()
+        FilePath(tempdir).makedirs()
 
-        site = TahoeLAFSSite(self.mktemp(), Resource(), logPath=logPath)
+        site = TahoeLAFSSite(
+            anonymous_tempfile_factory(tempdir),
+            Resource(),
+            logPath=logPath,
+        )
         site.startFactory()
 
         channel = DummyChannel()
@@ -257,11 +249,17 @@ def _create_request(self, tempdir):
         Create and return a new ``TahoeLAFSRequest`` hooked up to a
         ``TahoeLAFSSite``.
 
-        :param bytes tempdir: The temporary directory to give to the site.
+        :param FilePath tempdir: The temporary directory to configure the site
+            to write large temporary request bodies to.  The temporary files
+            will be named for ease of testing.
 
         :return TahoeLAFSRequest: The new request instance.
         """
-        site = TahoeLAFSSite(tempdir.path, Resource(), logPath=self.mktemp())
+        site = TahoeLAFSSite(
+            lambda: tempfile.NamedTemporaryFile(dir=tempdir.path),
+            Resource(),
+            logPath=self.mktemp(),
+        )
         site.startFactory()
 
         channel = DummyChannel()
@@ -275,6 +273,7 @@ def test_small_content(self, request_body_size):
         A request body smaller than 1 MiB is kept in memory.
         """
         tempdir = FilePath(self.mktemp())
+        tempdir.makedirs()
         request = self._create_request(tempdir)
         request.gotLength(request_body_size)
         self.assertThat(
@@ -284,57 +283,21 @@ def test_small_content(self, request_body_size):
 
     def _large_request_test(self, request_body_size):
         """
-        Assert that when a request with a body of of the given size is received
-        its content is written to the directory the ``TahoeLAFSSite`` is
-        configured with.
+        Assert that when a request with a body of the given size is
+        received its content is written a temporary file created by the given
+        tempfile factory.
         """
         tempdir = FilePath(self.mktemp())
         tempdir.makedirs()
         request = self._create_request(tempdir)
-
-        # So.  Bad news.  The temporary file for the uploaded content is
-        # unnamed (and this isn't even necessarily a bad thing since it is how
-        # you get automatic on-process-exit cleanup behavior on POSIX).  It's
-        # not visible by inspecting the filesystem.  It has no name we can
-        # discover.  Then how do we verify it is written to the right place?
-        # The question itself is meaningless if we try to be too precise.  It
-        # *has* no filesystem location.  However, it is still stored *on* some
-        # filesystem.  We still want to make sure it is on the filesystem we
-        # specified because otherwise it might be on a filesystem that's too
-        # small or undesirable in some other way.
-        #
-        # I don't know of any way to ask a file descriptor which filesystem
-        # it's on, either, though.  It might be the case that the [f]statvfs()
-        # result could be compared somehow to infer the filesystem but
-        # ... it's not clear what the failure modes might be there, across
-        # different filesystems and runtime environments.
-        #
-        # Another approach is to make the temp directory unwriteable and
-        # observe the failure when an attempt is made to create a file there.
-        # This is hardly a lovely solution but at least it's kind of simple.
-        #
-        # It would be nice if it worked consistently cross-platform but on
-        # Windows os.chmod is more or less broken.
-        if platform.isWindows():
-            request.gotLength(request_body_size)
-            self.assertThat(
-                tempdir.children(),
-                HasLength(1),
-            )
-        else:
-            tempdir.chmod(0o550)
-            with self.assertRaises(OSError) as ctx:
-                request.gotLength(request_body_size)
-                raise Exception(
-                    "OSError not raised, instead tempdir.children() = {}".format(
-                        tempdir.children(),
-                    ),
-                )
-
-            self.assertThat(
-                ctx.exception.errno,
-                Equals(EACCES),
-            )
+        request.gotLength(request_body_size)
+        # We can see the temporary file in the temporary directory we
+        # specified because _create_request makes a request that uses named
+        # temporary files instead of the usual anonymous temporary files.
+        self.assertThat(
+            tempdir.children(),
+            HasLength(1),
+        )
 
     def test_unknown_request_size(self):
         """

src/allmydata/webish.py

@@ -4,7 +4,7 @@
 from __future__ import annotations
 
 from six import ensure_str
-
+from typing import IO, Callable, Optional
 import re, time, tempfile
 from urllib.parse import parse_qsl, urlencode
 
@@ -217,36 +217,50 @@ def censor(queryargs: bytes) -> bytes:
     return urlencode(result, safe="[]").encode("ascii")
 
 
+def anonymous_tempfile_factory(tempdir: bytes) -> Callable[[], IO[bytes]]:
+    """
+    Create a no-argument callable for creating a new temporary file in the
+    given directory.
+
+    :param tempdir: The directory in which temporary files with be created.
+
+    :return: The callable.
+    """
+    return lambda: tempfile.TemporaryFile(dir=tempdir)
+
+
 class TahoeLAFSSite(Site, object):
     """
     The HTTP protocol factory used by Tahoe-LAFS.
 
     Among the behaviors provided:
 
-    * A configurable temporary directory where large request bodies can be
-      written so they don't stay in memory.
+    * A configurable temporary file factory for large request bodies to avoid
+      keeping them in memory.
 
     * A log formatter that writes some access logs but omits capability
       strings to help keep them secret.
     """
     requestFactory = TahoeLAFSRequest
 
-    def __init__(self, tempdir, *args, **kwargs):
+    def __init__(self, make_tempfile: Callable[[], IO[bytes]], *args, **kwargs):
         Site.__init__(self, *args, logFormatter=_logFormatter, **kwargs)
-        self._tempdir = tempdir
+        assert callable(make_tempfile)
+        with make_tempfile():
+            pass
+        self._make_tempfile = make_tempfile
 
-    def getContentFile(self, length):
+    def getContentFile(self, length: Optional[int]) -> IO[bytes]:
         if length is None or length >= 1024 * 1024:
-            return tempfile.TemporaryFile(dir=self._tempdir)
+            return self._make_tempfile()
         return BytesIO()
 
-
 class WebishServer(service.MultiService):
     # The type in Twisted for services is wrong in 22.10...
     # https://github.com/twisted/twisted/issues/10135
     name = "webish"  # type: ignore[assignment]
 
-    def __init__(self, client, webport, tempdir, nodeurl_path=None, staticdir=None,
+    def __init__(self, client, webport, make_tempfile, nodeurl_path=None, staticdir=None,
                  clock=None, now_fn=time.time):
         service.MultiService.__init__(self)
         # the 'data' argument to all render() methods default to the Client
@@ -256,7 +270,7 @@ def __init__(self, client, webport, tempdir, nodeurl_path=None, staticdir=None,
         # time in a deterministic manner.
 
         self.root = root.Root(client, clock, now_fn)
-        self.buildServer(webport, tempdir, nodeurl_path, staticdir)
+        self.buildServer(webport, make_tempfile, nodeurl_path, staticdir)
 
         # If set, clock is a twisted.internet.task.Clock that the tests
         # use to test ophandle expiration.
@@ -266,9 +280,9 @@ def __init__(self, client, webport, tempdir, nodeurl_path=None, staticdir=None,
 
         self.root.putChild(b"storage-plugins", StoragePlugins(client))
 
-    def buildServer(self, webport, tempdir, nodeurl_path, staticdir):
+    def buildServer(self, webport, make_tempfile, nodeurl_path, staticdir):
         self.webport = webport
-        self.site = TahoeLAFSSite(tempdir, self.root)
+        self.site = TahoeLAFSSite(make_tempfile, self.root)
         self.staticdir = staticdir # so tests can check
         if staticdir:
             self.root.putChild(b"static", static.File(staticdir))
@@ -346,4 +360,4 @@ class IntroducerWebishServer(WebishServer):
     def __init__(self, introducer, webport, nodeurl_path=None, staticdir=None):
         service.MultiService.__init__(self)
         self.root = introweb.IntroducerRoot(introducer)
-        self.buildServer(webport, tempfile.tempdir, nodeurl_path, staticdir)
+        self.buildServer(webport, tempfile.TemporaryFile, nodeurl_path, staticdir)