Merge pull request #243 from labthings/fallback-bugfixes

Fallback bugfixes

Author: julianstirling

src/labthings_fastapi/server/__init__.py

@@ -81,6 +81,7 @@ def __init__(
         :param settings_folder: the location on disk where `.Thing`
             settings will be saved.
         """
+        self.startup_failure: dict | None = None
         configure_thing_logger()  # Note: this is safe to call multiple times.
         self._config = ThingServerConfig(things=things, settings_folder=settings_folder)
         self.app = FastAPI(lifespan=self.lifespan)
@@ -259,6 +260,10 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None, None]:
         :param app: The FastAPI application wrapped by the server.
         :yield: no value. The FastAPI application will serve requests while this
             function yields.
+        :raises BaseException: Reraises any errors that are caught when calling
+            ``__enter__`` on each Thing. The error is also saved to
+            ``self.startup_failure`` for post mortem, as otherwise uvicorn will swallow
+            it and replace it with SystemExit(3) and no traceback.
         """
         async with BlockingPortal() as portal:
             # We create a blocking portal to allow threaded code to call async code
@@ -271,7 +276,14 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None, None]:
             # is present when this happens, in case we are dealing with threads.
             async with AsyncExitStack() as stack:
                 for thing in self.things.values():
-                    await stack.enter_async_context(thing)
+                    try:
+                        await stack.enter_async_context(thing)
+                    except BaseException as e:
+                        self.startup_failure = {
+                            "thing": thing.name,
+                            "exception": e,
+                        }
+                        raise
                 yield
 
         self.blocking_portal = None

src/labthings_fastapi/server/fallback.py

@@ -13,10 +13,10 @@
 from fastapi import FastAPI
 from fastapi.responses import HTMLResponse
 from starlette.responses import RedirectResponse
+from .config_model import ThingServerConfig
 
 if TYPE_CHECKING:
     from . import ThingServer
-    from .config_model import ThingServerConfig
 
 
 class FallbackApp(FastAPI):
@@ -32,7 +32,8 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
         :param \**kwargs: is passed to `fastapi.FastAPI.__init__`\ .
         """
         super().__init__(*args, **kwargs)
-        self.labthings_config: ThingServerConfig | None = None
+        # Handle dictionary config here for legacy reasons.
+        self.labthings_config: ThingServerConfig | dict | None = None
         self.labthings_server: ThingServer | None = None
         self.labthings_error: BaseException | None = None
         self.log_history = None
@@ -79,19 +80,22 @@ async def root() -> HTMLResponse:
 
     :return: a response that serves the error as an HTML page.
     """
-    error_message = f"{app.labthings_error}"
-    # use traceback.format_exception to get full traceback as list
-    # this ends in newlines, but needs joining to be a single string
-    error_w_trace = "".join(format_exception(app.labthings_error))
+    error_message, error_w_trace = _format_error_and_traceback()
     things = ""
     if app.labthings_server:
         for path, thing in app.labthings_server.things.items():
             things += f"<li>{path}: {thing!r}</li>"
 
+    config = app.labthings_config
+    if isinstance(config, ThingServerConfig):
+        conf_str = config.model_dump_json(indent=2)
+    else:
+        conf_str = json.dumps(config, indent=2)
+
     content = ERROR_PAGE
     content = content.replace("{{error}}", error_message)
     content = content.replace("{{things}}", things)
-    content = content.replace("{{config}}", json.dumps(app.labthings_config, indent=2))
+    content = content.replace("{{config}}", conf_str)
     content = content.replace("{{traceback}}", error_w_trace)
 
     if app.log_history is None:
@@ -103,6 +107,34 @@ async def root() -> HTMLResponse:
     return HTMLResponse(content=content, status_code=app.html_code)
 
 
+def _format_error_and_traceback() -> tuple[str, str]:
+    """Format the error and traceback.
+
+    If the error was in lifespan causing Uvicorn to raise SystemExit(3) without a
+    traceback. Try to extract the saved exception from the server.
+
+    :return: A tuple of error message and error traceback.
+    """
+    err = app.labthings_error
+    server = app.labthings_server
+    error_message = f"{err}"
+
+    if (
+        isinstance(err, SystemExit)
+        and server is not None
+        and isinstance(server.startup_failure, dict)
+    ):
+        # It is a uvicorn SystemExit, so replace err with the saved error in the server.
+        err = server.startup_failure.get("exception", err)
+        thing = server.startup_failure.get("thing", "Unknown")
+        error_message = f"Failed to enter '{thing}' Thing: {err}"
+
+    # use traceback.format_exception to get full traceback as list
+    # this ends in newlines, but needs joining to be a single string
+    error_w_trace = "".join(format_exception(err))
+    return error_message, error_w_trace
+
+
 @app.get("/{path:path}")
 async def redirect_to_root(path: str) -> RedirectResponse:
     """Redirect all paths on the server to the error page.

tests/test_fallback.py

@@ -5,9 +5,56 @@
 verifies that it works as expected.
 """
 
+import re
+
+import pytest
+import uvicorn
+
 from fastapi.testclient import TestClient
 import labthings_fastapi as lt
 from labthings_fastapi.server.fallback import app
+from labthings_fastapi.example_things import ThingThatCantStart
+
+CONFIG_DICT = {
+    "things": {
+        "thing1": "labthings_fastapi.example_things:MyThing",
+        "thing2": {
+            "class": "labthings_fastapi.example_things:MyThing",
+            "kwargs": {},
+        },
+    }
+}
+
+
+@pytest.fixture(autouse=True)
+def reset_app_state():
+    """Reset the fallback app state before each fallback test."""
+    app.labthings_config = None
+    app.labthings_server = None
+    app.labthings_error = None
+    app.log_history = None
+
+
+def test_fallback_redirect():
+    """Test that the redirect works."""
+    with TestClient(app) as client:
+        response = client.get("/")
+        # No history as no redirect
+        assert len(response.history) == 0
+        html = response.text
+        # test that something when wrong is shown
+        assert "Something went wrong" in html
+
+        # Now try another url
+        response = client.get("/foo/bar")
+        # redirected so there is a history item showing a 307 Temporary Redirect code.
+        assert len(response.history) == 1
+        assert response.history[0].status_code == 307
+
+        # Redirects to error page.
+        html = response.text
+        # test that something when wrong is shown
+        assert "Something went wrong" in html
 
 
 def test_fallback_empty():
@@ -19,14 +66,31 @@ def test_fallback_empty():
         assert "No logging info available" in html
 
 
-def test_fallback_with_config():
-    app.labthings_config = {"hello": "goodbye"}
+def test_fallback_with_config_dict():
+    """Check that fallback server prints a config dictionary as JSON."""
+    app.labthings_config = CONFIG_DICT
+    with TestClient(app) as client:
+        response = client.get("/")
+        html = response.text
+        assert "Something went wrong" in html
+        assert "No logging info available" in html
+        assert '"thing1": "labthings_fastapi.example_things:MyThing"' in html
+        assert '"class": "labthings_fastapi.example_things:MyThing"' in html
+
+
+def test_fallback_with_config_obj():
+    """Check that fallback server prints the config object as JSON."""
+    config = lt.ThingServerConfig.model_validate(CONFIG_DICT)
+    app.labthings_config = config
     with TestClient(app) as client:
         response = client.get("/")
         html = response.text
         assert "Something went wrong" in html
         assert "No logging info available" in html
-        assert '"hello": "goodbye"' in html
+        assert "thing1" in html
+        assert "thing2" in html
+        cls_regex = re.compile(r'"cls": "labthings_fastapi\.example_things\.MyThing"')
+        assert len(cls_regex.findall(html)) == 2
 
 
 def test_fallback_with_error():
@@ -41,16 +105,7 @@ def test_fallback_with_error():
 
 
 def test_fallback_with_server():
-    config_dict = {
-        "things": {
-            "thing1": "labthings_fastapi.example_things:MyThing",
-            "thing2": {
-                "class": "labthings_fastapi.example_things:MyThing",
-                "kwargs": {},
-            },
-        }
-    }
-    config = lt.ThingServerConfig.model_validate(config_dict)
+    config = lt.ThingServerConfig.model_validate(CONFIG_DICT)
     app.labthings_server = lt.ThingServer.from_config(config)
     with TestClient(app) as client:
         response = client.get("/")
@@ -70,3 +125,38 @@ def test_fallback_with_log():
         assert "No logging info available" not in html
         assert "<p>Logging info</p>" in html
         assert "Fake log content" in html
+
+
+def test_actual_server_fallback():
+    """Test that the the server configures its startup failure correctly.
+
+    This may want to become an integration test in the fullness of time. Though
+    the integration test may want to actually let the cli really serve up the
+    fallback.
+    """
+    # ThingThatCantStart has an error in __enter__
+    server = lt.ThingServer({"bad_thing": ThingThatCantStart})
+
+    # Starting the server is a SystemExit
+    with pytest.raises(SystemExit, match="3") as excinfo:
+        uvicorn.run(server.app, port=5000)
+    server_error = excinfo.value
+    assert server.startup_failure is not None
+    assert server.startup_failure["thing"] == "bad_thing"
+    thing_error = server.startup_failure["exception"]
+    assert isinstance(thing_error, RuntimeError)
+
+    app.labthings_server = server
+    app.labthings_error = server_error
+    with TestClient(app) as client:
+        response = client.get("/")
+        html = response.text
+        assert "Something went wrong" in html
+        # Shouldn't be displaying the meaningless SystemExit
+        assert "SystemExit" not in html
+
+        # The message from when the Thing errored should be displayed
+        assert str(thing_error) in html
+        # With the traceback
+        assert 'labthings_fastapi/example_things/__init__.py", line' in html
+        assert f'RuntimeError("{thing_error}")' in html