From 60e0d8d4c131ffa17ea204da5457ddb72d658085 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Wed, 31 Jan 2024 13:10:14 +0000 Subject: [PATCH] Fix CSS watcher crashing when file becomes unavailable... (#4079) * Managing exceptions when watched CSS files are unavailable * Handling scenario where FileMonitor crashes when file temporarily becomes unavailable. * Update CHANGELOG * Update log level to warning --- CHANGELOG.md | 1 + src/textual/app.py | 11 +++++++- src/textual/file_monitor.py | 9 +++++- tests/css/test_css_reloading.py | 29 +++++++++++++++---- tests/test_file_monitor.py | 50 +++++++++++++++++++++++++++++++++ 5 files changed, 93 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1844edd8e..8bb59c36a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Ensuring `TextArea.SelectionChanged` message only sends when the updated selection is different https://github.com/Textualize/textual/pull/3933 - Fixed declaration after nested rule set causing a parse error https://github.com/Textualize/textual/pull/4012 - ID and class validation was too lenient https://github.com/Textualize/textual/issues/3954 +- Fixed CSS watcher crash if file becomes unreadable (even temporarily) https://github.com/Textualize/textual/pull/4079 - Fixed display of keys when used in conjunction with other keys https://github.com/Textualize/textual/pull/3050 ## [0.47.1] - 2023-01-05 diff --git a/src/textual/app.py b/src/textual/app.py index 072625b0c..f1290d088 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -72,6 +72,7 @@ from .actions import ActionParseResult, SkipAction from .await_remove import AwaitRemove from .binding import Binding, BindingType, _Bindings from .command import CommandPalette, Provider +from .css.errors import StylesheetError from .css.query import NoMatches from .css.stylesheet import RulesMap, Stylesheet from .design import ColorSystem @@ -1469,7 +1470,15 @@ class App(Generic[ReturnType], DOMNode): try: time = perf_counter() stylesheet = self.stylesheet.copy() - stylesheet.read_all(css_paths) + try: + stylesheet.read_all(css_paths) + except StylesheetError as error: + # If one of the CSS paths is no longer available (or perhaps temporarily unavailable), + # we'll end up with partial CSS, which is probably confusing more than anything. We opt to do + # nothing here, knowing that we'll retry again very soon, on the next file monitor invocation. + # Related issue: https://github.com/Textualize/textual/issues/3996 + self.log.warning(str(error)) + return stylesheet.parse() elapsed = (perf_counter() - time) * 1000 if self._css_has_errors: diff --git a/src/textual/file_monitor.py b/src/textual/file_monitor.py index 3f7bbb3fc..36a4180bb 100644 --- a/src/textual/file_monitor.py +++ b/src/textual/file_monitor.py @@ -31,7 +31,14 @@ class FileMonitor: def _get_last_modified_time(self) -> float: """Get the most recent modified time out of all files being watched.""" - return max((os.stat(path).st_mtime for path in self._paths), default=0) + modified_times = [] + for path in self._paths: + try: + modified_time = os.stat(path).st_mtime + except FileNotFoundError: + modified_time = 0 + modified_times.append(modified_time) + return max(modified_times, default=0) def check(self) -> bool: """Check the monitored files. Return True if any were changed since the last modification time.""" diff --git a/tests/css/test_css_reloading.py b/tests/css/test_css_reloading.py index e69060415..b572e97dc 100644 --- a/tests/css/test_css_reloading.py +++ b/tests/css/test_css_reloading.py @@ -1,7 +1,4 @@ -""" -Regression test for https://github.com/Textualize/textual/issues/3931 -""" - +import os from pathlib import Path from textual.app import App, ComposeResult @@ -34,7 +31,7 @@ class MyApp(App[None]): async def test_css_reloading_applies_to_non_top_screen(monkeypatch) -> None: # type: ignore - """Regression test for https://github.com/Textualize/textual/issues/2063.""" + """Regression test for https://github.com/Textualize/textual/issues/3931""" monkeypatch.setenv( "TEXTUAL", "debug" @@ -65,3 +62,25 @@ Label { # Height should fall back to 1. assert first_label.styles.height is not None assert first_label.styles.height.value == 1 + + +async def test_css_reloading_file_not_found(monkeypatch, tmp_path): + """Regression test for https://github.com/Textualize/textual/issues/3996 + + Files can become temporarily unavailable during saving on certain environments. + """ + monkeypatch.setenv("TEXTUAL", "debug") + + css_path = tmp_path / "test_css_reloading_file_not_found.tcss" + with open(css_path, "w") as css_file: + css_file.write("#a {color: red;}") + + class TextualApp(App): + CSS_PATH = css_path + + app = TextualApp() + async with app.run_test() as pilot: + await pilot.app._on_css_change() + os.remove(css_path) + assert not css_path.exists() + await pilot.app._on_css_change() diff --git a/tests/test_file_monitor.py b/tests/test_file_monitor.py index f6c384968..fc93c7d29 100644 --- a/tests/test_file_monitor.py +++ b/tests/test_file_monitor.py @@ -1,3 +1,4 @@ +import os from pathlib import Path from textual.file_monitor import FileMonitor @@ -6,3 +7,52 @@ from textual.file_monitor import FileMonitor def test_repr() -> None: file_monitor = FileMonitor([Path(".")], lambda: None) assert "FileMonitor" in repr(file_monitor) + + +def test_file_never_found(): + path = "doesnt_exist.tcss" + file_monitor = FileMonitor([Path(path)], lambda: None) + file_monitor.check() # Ensuring no exceptions are raised. + + +async def test_file_deletion(tmp_path): + """In some environments, a file can become temporarily unavailable during saving. + + This test ensures the FileMonitor is robust enough to handle a file temporarily being + unavailable, and that it recovers when the file becomes available again. + + Regression test for: https://github.com/Textualize/textual/issues/3996 + """ + + def make_file(): + path = tmp_path / "will_be_deleted.tcss" + path.write_text("#foo { color: dodgerblue; }") + return path + + callback_counter = 0 + + def callback(): + nonlocal callback_counter + callback_counter += 1 + + path = make_file() + file_monitor = FileMonitor([path], callback) + + # Ensure the file monitor survives after the file is gone + await file_monitor() + os.remove(path) + + # The file is now unavailable, but we don't crash here + await file_monitor() + await file_monitor() + + # Despite multiple checks, the file was only available for the first check, + # and we only fire the callback while the file is available. + assert callback_counter == 1 + + # The file is made available again. + make_file() + + # Since the file is available, the callback should fire when the FileMonitor is called + await file_monitor() + assert callback_counter == 2