From 4b23b779e412797214736f6421d1776210d86738 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 7 Jul 2023 16:00:00 -0400 Subject: [PATCH 1/5] Reduce the amount of test suite gymnastics with new WebishServer API Instead of forcing the test suite to try to discover the location of an unnamed temporary file, let it just assert that the file is created in the directory specified in the temporary file factory. --- src/allmydata/client.py | 7 ++- src/allmydata/test/web/test_web.py | 2 +- src/allmydata/test/web/test_webish.py | 89 +++++++++------------------ src/allmydata/webish.py | 40 ++++++++---- 4 files changed, 61 insertions(+), 77 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index e85ed4fe2..fefd29657 100644 --- a/src/allmydata/client.py +++ b/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 @@ -7,6 +7,7 @@ import os import stat import time import weakref +import tempfile from typing import Optional, Iterable from base64 import urlsafe_b64encode from functools import partial @@ -1029,14 +1030,14 @@ class _Client(node.Node, pollmixin.PollMixin): 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 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(self._get_tempdir()), nodeurl_path, staticdir, ) diff --git a/src/allmydata/test/web/test_web.py b/src/allmydata/test/web/test_web.py index 08dce0ac0..bd7ca3f7f 100644 --- a/src/allmydata/test/web/test_web.py +++ b/src/allmydata/test/web/test_web.py @@ -341,7 +341,7 @@ class WebMixin(TimezoneMixin): self.ws = webish.WebishServer( self.s, "0", - tempdir=tempdir.path, + webish.anonymous_tempfile(tempdir.path), staticdir=self.staticdir, clock=self.clock, now_fn=lambda:self.fakeTime, diff --git a/src/allmydata/test/web/test_webish.py b/src/allmydata/test/web/test_webish.py index 050f77d1c..d444df436 100644 --- a/src/allmydata/test/web/test_webish.py +++ b/src/allmydata/test/web/test_webish.py @@ -1,17 +1,8 @@ """ 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, ) @@ -59,6 +50,7 @@ from ..common import ( from ...webish import ( TahoeLAFSRequest, TahoeLAFSSite, + anonymous_tempfile, ) @@ -183,8 +175,14 @@ class TahoeLAFSSiteTests(SyncTestCase): :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(tempdir), + Resource(), + logPath=logPath, + ) site.startFactory() channel = DummyChannel() @@ -257,11 +255,17 @@ class TahoeLAFSSiteTests(SyncTestCase): 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 +279,7 @@ class TahoeLAFSSiteTests(SyncTestCase): 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 +289,21 @@ class TahoeLAFSSiteTests(SyncTestCase): 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): """ diff --git a/src/allmydata/webish.py b/src/allmydata/webish.py index ec2582f80..2bae7f8a5 100644 --- a/src/allmydata/webish.py +++ b/src/allmydata/webish.py @@ -4,7 +4,7 @@ General web server-related utilities. from __future__ import annotations from six import ensure_str - +from typing import BinaryIO, 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(tempdir: bytes) -> BinaryIO: + """ + 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[[], BinaryIO], *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]) -> BinaryIO: 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 @@ class WebishServer(service.MultiService): # 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 @@ class WebishServer(service.MultiService): 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) From 3129898563ed42cc8266d7b3c5d3aad43c9d3f56 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 7 Jul 2023 16:04:54 -0400 Subject: [PATCH 2/5] news fragment --- newsfragments/4044.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/4044.minor diff --git a/newsfragments/4044.minor b/newsfragments/4044.minor new file mode 100644 index 000000000..e69de29bb From c838967a54635f4feb9ac2209e7c5e8d2b374f62 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Tue, 11 Jul 2023 16:15:56 -0400 Subject: [PATCH 3/5] Improve the name and type annotation of the tempfile factory --- src/allmydata/client.py | 4 ++-- src/allmydata/test/web/test_web.py | 2 +- src/allmydata/test/web/test_webish.py | 4 ++-- src/allmydata/webish.py | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index fefd29657..13909d5ca 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -1030,14 +1030,14 @@ class _Client(node.Node, pollmixin.PollMixin): def init_web(self, webport): self.log("init_web(webport=%s)", args=(webport,)) - from allmydata.webish import WebishServer, anonymous_tempfile + 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, - anonymous_tempfile(self._get_tempdir()), + anonymous_tempfile_factory(self._get_tempdir()), nodeurl_path, staticdir, ) diff --git a/src/allmydata/test/web/test_web.py b/src/allmydata/test/web/test_web.py index bd7ca3f7f..42be0f50d 100644 --- a/src/allmydata/test/web/test_web.py +++ b/src/allmydata/test/web/test_web.py @@ -341,7 +341,7 @@ class WebMixin(TimezoneMixin): self.ws = webish.WebishServer( self.s, "0", - webish.anonymous_tempfile(tempdir.path), + webish.anonymous_tempfile_factory(tempdir.path), staticdir=self.staticdir, clock=self.clock, now_fn=lambda:self.fakeTime, diff --git a/src/allmydata/test/web/test_webish.py b/src/allmydata/test/web/test_webish.py index d444df436..a0206c7eb 100644 --- a/src/allmydata/test/web/test_webish.py +++ b/src/allmydata/test/web/test_webish.py @@ -50,7 +50,7 @@ from ..common import ( from ...webish import ( TahoeLAFSRequest, TahoeLAFSSite, - anonymous_tempfile, + anonymous_tempfile_factory, ) @@ -179,7 +179,7 @@ class TahoeLAFSSiteTests(SyncTestCase): FilePath(tempdir).makedirs() site = TahoeLAFSSite( - anonymous_tempfile(tempdir), + anonymous_tempfile_factory(tempdir), Resource(), logPath=logPath, ) diff --git a/src/allmydata/webish.py b/src/allmydata/webish.py index 2bae7f8a5..2a55b3446 100644 --- a/src/allmydata/webish.py +++ b/src/allmydata/webish.py @@ -217,7 +217,7 @@ def censor(queryargs: bytes) -> bytes: return urlencode(result, safe="[]").encode("ascii") -def anonymous_tempfile(tempdir: bytes) -> BinaryIO: +def anonymous_tempfile_factory(tempdir: bytes) -> Callable[[], BinaryIO]: """ Create a no-argument callable for creating a new temporary file in the given directory. From 79512a93e722f95e75ff63b07df0177c2781d464 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Tue, 11 Jul 2023 16:30:54 -0400 Subject: [PATCH 4/5] Adjust the temp factory return type BinaryIO is a subclass of IO[bytes] so it doesn't check out as the return type of a callable we pass around. Switch to the superclass instead. --- src/allmydata/webish.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/allmydata/webish.py b/src/allmydata/webish.py index 2a55b3446..ec5ad64c0 100644 --- a/src/allmydata/webish.py +++ b/src/allmydata/webish.py @@ -4,7 +4,7 @@ General web server-related utilities. from __future__ import annotations from six import ensure_str -from typing import BinaryIO, Callable, Optional +from typing import IO, Callable, Optional import re, time, tempfile from urllib.parse import parse_qsl, urlencode @@ -217,7 +217,7 @@ def censor(queryargs: bytes) -> bytes: return urlencode(result, safe="[]").encode("ascii") -def anonymous_tempfile_factory(tempdir: bytes) -> Callable[[], BinaryIO]: +def anonymous_tempfile_factory(tempdir: bytes) -> Callable[[], IO[bytes]]: """ Create a no-argument callable for creating a new temporary file in the given directory. @@ -243,14 +243,14 @@ class TahoeLAFSSite(Site, object): """ requestFactory = TahoeLAFSRequest - def __init__(self, make_tempfile: Callable[[], BinaryIO], *args, **kwargs): + def __init__(self, make_tempfile: Callable[[], IO[bytes]], *args, **kwargs): Site.__init__(self, *args, logFormatter=_logFormatter, **kwargs) assert callable(make_tempfile) with make_tempfile(): pass self._make_tempfile = make_tempfile - def getContentFile(self, length: Optional[int]) -> BinaryIO: + def getContentFile(self, length: Optional[int]) -> IO[bytes]: if length is None or length >= 1024 * 1024: return self._make_tempfile() return BytesIO() From eef52fa59fa64f166fc1484653abefac84a6dff7 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Tue, 11 Jul 2023 16:32:33 -0400 Subject: [PATCH 5/5] remove unused imports --- src/allmydata/client.py | 1 - src/allmydata/test/web/test_webish.py | 6 ------ 2 files changed, 7 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index 13909d5ca..a1501f1ef 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -7,7 +7,6 @@ import os import stat import time import weakref -import tempfile from typing import Optional, Iterable from base64 import urlsafe_b64encode from functools import partial diff --git a/src/allmydata/test/web/test_webish.py b/src/allmydata/test/web/test_webish.py index a0206c7eb..523dfc878 100644 --- a/src/allmydata/test/web/test_webish.py +++ b/src/allmydata/test/web/test_webish.py @@ -6,9 +6,6 @@ import tempfile from uuid import ( uuid4, ) -from errno import ( - EACCES, -) from io import ( BytesIO, ) @@ -30,9 +27,6 @@ from testtools.matchers import ( HasLength, ) -from twisted.python.runtime import ( - platform, -) from twisted.python.filepath import ( FilePath, )