From 72018cc26bde283075208680286f028b281d033f Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Thu, 18 Apr 2024 10:44:46 -0700 Subject: [PATCH] timeout: Rename to unsafe_timeout. This timeout strategy using asynchronous exceptions has a number of safety caveats (read the docstring!!) and should only be used in very specific circumstances. Signed-off-by: Anders Kaseorg --- zerver/lib/markdown/__init__.py | 4 ++-- zerver/lib/test_classes.py | 2 +- zerver/lib/timeout.py | 2 +- zerver/tests/test_markdown.py | 2 +- zerver/tests/test_timeout.py | 10 +++++----- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/zerver/lib/markdown/__init__.py b/zerver/lib/markdown/__init__.py index fb23f19e6b..451bef9d29 100644 --- a/zerver/lib/markdown/__init__.py +++ b/zerver/lib/markdown/__init__.py @@ -68,7 +68,7 @@ from zerver.lib.outgoing_http import OutgoingSession from zerver.lib.subdomains import is_static_or_current_realm_url from zerver.lib.tex import render_tex from zerver.lib.thumbnail import user_uploads_or_external -from zerver.lib.timeout import timeout +from zerver.lib.timeout import unsafe_timeout from zerver.lib.timezone import common_timezones from zerver.lib.types import LinkifierDict from zerver.lib.url_encoding import encode_stream, hash_util_encode @@ -2667,7 +2667,7 @@ def do_convert( # extremely inefficient in corner cases) as well as user # errors (e.g. a linkifier that makes some syntax # infinite-loop). - rendering_result.rendered_content = timeout(5, lambda: _md_engine.convert(content)) + rendering_result.rendered_content = unsafe_timeout(5, lambda: _md_engine.convert(content)) # Throw an exception if the content is huge; this protects the # rest of the codebase from any bugs where we end up rendering diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index 1ba5be0350..67cd1ad334 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -1560,7 +1560,7 @@ Output: markdown.__init__.do_convert. """ with mock.patch( - "zerver.lib.markdown.timeout", side_effect=subprocess.CalledProcessError(1, []) + "zerver.lib.markdown.unsafe_timeout", side_effect=subprocess.CalledProcessError(1, []) ), self.assertLogs(level="ERROR"): # For markdown_logger.exception yield diff --git a/zerver/lib/timeout.py b/zerver/lib/timeout.py index dba89cc74f..43278ee655 100644 --- a/zerver/lib/timeout.py +++ b/zerver/lib/timeout.py @@ -22,7 +22,7 @@ class TimeoutExpiredError(Exception): ResultT = TypeVar("ResultT") -def timeout(timeout: float, func: Callable[[], ResultT]) -> ResultT: +def unsafe_timeout(timeout: float, func: Callable[[], ResultT]) -> ResultT: """Call the function in a separate thread. Return its return value, or raise an exception, within approximately 'timeout' seconds. diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index b50a511f24..9a75e9374a 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -3305,7 +3305,7 @@ class MarkdownErrorTests(ZulipTestCase): throws an exception""" msg = "mock rendered message\n" * 10 * settings.MAX_MESSAGE_LENGTH - with mock.patch("zerver.lib.markdown.timeout", return_value=msg), mock.patch( + with mock.patch("zerver.lib.markdown.unsafe_timeout", return_value=msg), mock.patch( "zerver.lib.markdown.markdown_logger" ): with self.assertRaises(MarkdownRenderingError): diff --git a/zerver/tests/test_timeout.py b/zerver/tests/test_timeout.py index b03f94ab62..acaa5c77a1 100644 --- a/zerver/tests/test_timeout.py +++ b/zerver/tests/test_timeout.py @@ -4,7 +4,7 @@ import traceback from unittest import skipIf from zerver.lib.test_classes import ZulipTestCase -from zerver.lib.timeout import TimeoutExpiredError, timeout +from zerver.lib.timeout import TimeoutExpiredError, unsafe_timeout class TimeoutTestCase(ZulipTestCase): @@ -20,12 +20,12 @@ class TimeoutTestCase(ZulipTestCase): return 42 # nocoverage def test_timeout_returns(self) -> None: - ret = timeout(1, lambda: 42) + ret = unsafe_timeout(1, lambda: 42) self.assertEqual(ret, 42) def test_timeout_exceeded(self) -> None: try: - timeout(1, lambda: self.sleep_x_seconds_y_times(0.1, 50)) + unsafe_timeout(1, lambda: self.sleep_x_seconds_y_times(0.1, 50)) raise AssertionError("Failed to raise a timeout") except TimeoutExpiredError as exc: tb = traceback.format_tb(exc.__traceback__) @@ -34,7 +34,7 @@ class TimeoutTestCase(ZulipTestCase): def test_timeout_raises(self) -> None: try: - timeout(1, self.something_exceptional) + unsafe_timeout(1, self.something_exceptional) raise AssertionError("Failed to raise an exception") except ValueError as exc: tb = traceback.format_tb(exc.__traceback__) @@ -47,7 +47,7 @@ class TimeoutTestCase(ZulipTestCase): # kill it with self.assertLogs(level="WARNING") as m: try: - timeout(1, lambda: self.sleep_x_seconds_y_times(5, 1)) + unsafe_timeout(1, lambda: self.sleep_x_seconds_y_times(5, 1)) raise AssertionError("Failed to raise a timeout") except TimeoutExpiredError as exc: tb = traceback.format_tb(exc.__traceback__)