diff --git a/CHANGELOG.md b/CHANGELOG.md index dcce7c52..4ecd69f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Bug fixes +* A widget that fails after its render function returns (e.g., an Altair/VegaFusion chart without `vl-convert-python` installed) now shows the user an error notification instead of leaving the output blank. (#320) * Fixed many issues with widgets nested inside other widgets and with widget outputs inserted dynamically (e.g., `output_widget()` inside `@render.ui`). With ipywidgets 8, these either didn't render at all (e.g., a Plotly `FigureWidget` or a styled `HTML` widget inside an ipyleaflet popup) or rendered while logging errors like `Class HTMLStyleModel not found` (e.g., ipyleaflet popups, `WidgetControl` children, and leafmap legends). (#75, #101, #137, #157, #194, #237) * Re-rendering, replacing, hiding/showing, or removing widget outputs no longer leaks memory on the server or in the browser. Re-rendered widgets, and the data they hold, are now freed right away instead of when the session ends. (#246, #258, #260, #261, #262, #263, #303, #405) Related fixes: * pythreejs widgets no longer cause Shiny sessions to hang indefinitely with ipywidgets 8. (#244) diff --git a/shinywidgets/_comm.py b/shinywidgets/_comm.py index 3998d74d..9c074907 100644 --- a/shinywidgets/_comm.py +++ b/shinywidgets/_comm.py @@ -6,7 +6,7 @@ from dataclasses import dataclass, field from typing import TYPE_CHECKING, Callable, Dict, List, Optional -from shiny import Session +from shiny import Session, ui if TYPE_CHECKING: from ipywidgets import Widget @@ -258,6 +258,8 @@ def dispatch_client_msg( traitlets_level_patch.restore() scope.active = False handling_client_msg.reset(token) + if scope.errors: + notify_client_msg_error(self, scope.errors[0]) def handle_close(self, msg: Dict[str, object]) -> None: """The browser closed this comm: a later close() has nothing to publish.""" @@ -513,6 +515,13 @@ class ClientMsgScope: active = True + def __init__(self) -> None: + # The callback errors rerouted during dispatch, for its user-facing + # notification (see notify_client_msg_error). Their messages, not the + # exceptions: deferred work can outlive the dispatch holding this + # scope, and a traceback's frames would pin the widget and session. + self.errors: List[str] = [] + handling_client_msg: ContextVar[Optional[ClientMsgScope]] = ContextVar( "shinywidgets_handling_client_msg", default=None @@ -562,6 +571,27 @@ def restore(self) -> None: traitlets_level_patch = TraitletsLevelPatch() +def notify_client_msg_error(comm: ShinyComm, msg: str) -> None: + """ + Show a sticky browser notification for a widget-callback error that + ipywidgets swallowed (see ClientMsgErrorFilter): the error already + reached the server log, but if the failing callback is part of how the + widget renders, the user's output is otherwise blank with no + explanation. The browser replaces a notification whose id it already + shows, so a callback that fails on every message leaves one per widget. + """ + session = comm.session + if session.app.sanitize_errors: + msg = session.app.sanitize_error_msg + ui.notification_show( + msg, + type="error", + duration=None, + id=f"shinywidgets-error-{comm.comm_id}", + session=session, + ) + + class ClientMsgErrorFilter(logging.Filter): """ Outside IPython, ipywidgets catches exceptions raised by widget callbacks @@ -571,6 +601,9 @@ class ClientMsgErrorFilter(logging.Filter): While handling a browser message, move such records to the `shinywidgets` logger (dropping the original, so a configured root handler doesn't show them twice). Everywhere else, e.g. in Jupyter, `traitlets` logging is untouched. + + A rerouted exception is also noted on the dispatch's scope, so the + dispatch can show it to the user (see notify_client_msg_error). """ def filter(self, record: logging.LogRecord) -> bool: @@ -578,6 +611,9 @@ def filter(self, record: logging.LogRecord) -> bool: if scope is None or not scope.active or record.levelno < logging.WARNING: return True logger.error(record.getMessage(), exc_info=record.exc_info) + exc = record.exc_info[1] if record.exc_info else None + if exc is not None: + scope.errors.append(str(exc)) return False diff --git a/shinywidgets/_shinywidgets.py b/shinywidgets/_shinywidgets.py index 3f7cf60d..c52ebeb8 100644 --- a/shinywidgets/_shinywidgets.py +++ b/shinywidgets/_shinywidgets.py @@ -43,6 +43,7 @@ get_current_context, # pyright: ignore[reportPrivateImportUsage] ) from shiny.session import get_current_session, require_active_session, session_context +from shiny.types import NotifyException from ._accessors import rate_policy_for from ._as_widget import as_widget, pydeck_has_widget @@ -250,59 +251,78 @@ def _open_shiny_comm(): if w is None or WIDGET_INSTANCE_MAP.get(id) is not w: return - if is_ipyreact_registry_widget(w): - fan_out_ipyreact_registry(w) - return + try: + if is_ipyreact_registry_widget(w): + fan_out_ipyreact_registry(w) + return - # Already opened in another session by open_shared_in(); a plain - # ShinyComm here would cut that session off. - if isinstance(w.comm, SharedShinyComm): - open_shared_in(w, session) - return + # Already opened in another session by open_shared_in(); a plain + # ShinyComm here would cut that session off. + if isinstance(w.comm, SharedShinyComm): + open_shared_in(w, session) + return + + # A shared widget's lazily created defaults (e.g. its Layout) are shared too + with ( + shared_construction() + if shared + else WidgetRenderContext.resumed(render_ctx) + ): + prepare_to_serialize(w) + # Serve geolibre's bundled app from the Shiny app and switch the + # widget to the mode that probes that route. This must happen here + # (not in init_shiny_widget()): that hook runs inside + # Widget.__init__(), and geolibre's Map.__init__() sets + # _remote_mode after super().__init__() returns, overwriting it. + if any(widget_pkg(cls) == "geolibre" for cls in type(w).__mro__): + geolibre_app_mount(w, session) + # Mount a guarded loopback proxy for localtileserver-backed + # ipyleaflet layers and route their tile URLs through it (no-op + # for other widgets). Must run before get_state() so the browser + # only ever sees the rewritten (same-origin) URL. + localtileserver_layer_proxy(w, session) + state, buffer_paths, buffers = _remove_buffers(w.get_state()) + + # Covers widgets that are opened but never displayed (so no render-time + # walk opens their references); the browser would otherwise wait for a + # model it never gets. Foreign references are left for the render to reject. + for r in direct_widget_references(w): + if is_process_global(r): + open_shared_in(r, session) - # A shared widget's lazily created defaults (e.g. its Layout) are shared too - with ( - shared_construction() if shared else WidgetRenderContext.resumed(render_ctx) - ): - prepare_to_serialize(w) - # Serve geolibre's bundled app from the Shiny app and switch the - # widget to the mode that probes that route. This must happen here - # (not in init_shiny_widget()): that hook runs inside - # Widget.__init__(), and geolibre's Map.__init__() sets - # _remote_mode after super().__init__() returns, overwriting it. - if any(widget_pkg(cls) == "geolibre" for cls in type(w).__mro__): - geolibre_app_mount(w, session) - # Mount a guarded loopback proxy for localtileserver-backed - # ipyleaflet layers and route their tile URLs through it (no-op - # for other widgets). Must run before get_state() so the browser - # only ever sees the rewritten (same-origin) URL. - localtileserver_layer_proxy(w, session) - state, buffer_paths, buffers = _remove_buffers(w.get_state()) - - # Covers widgets that are opened but never displayed (so no render-time - # walk opens their references); the browser would otherwise wait for a - # model it never gets. Foreign references are left for the render to reject. - for r in direct_widget_references(w): - if is_process_global(r): - open_shared_in(r, session) - - # Initialize the comm -- this sends widget state to the frontend - placeholder = w.comm - w.comm = ShinyComm( - comm_id=id, - comm_manager=COMM_MANAGERS[session.id], - session=session, - target_name="jupyter.widget", - data={"state": state, "buffer_paths": buffer_paths}, - buffers=cast(BufferType, buffers), - # TODO: should this be hard-coded? - metadata={"version": __protocol_version__}, - html_deps=session._process_ui(TagList(*widget_deps))["deps"], - **open_keys, - ) + # Initialize the comm -- this sends widget state to the frontend + placeholder = w.comm + w.comm = ShinyComm( + comm_id=id, + comm_manager=COMM_MANAGERS[session.id], + session=session, + target_name="jupyter.widget", + data={"state": state, "buffer_paths": buffer_paths}, + buffers=cast(BufferType, buffers), + # TODO: should this be hard-coded? + metadata={"version": __protocol_version__}, + html_deps=session._process_ui(TagList(*widget_deps))["deps"], + **open_keys, + ) - flush_ipycanvas_manager(w) - flush_queued_sends(placeholder, w.comm) + flush_ipycanvas_manager(w) + flush_queued_sends(placeholder, w.comm) + except Exception as e: + # The render already returned, so Shiny's render-error display + # never sees this failure; unhandled, the output stays blank + # forever. NotifyException makes Shiny print the traceback and + # show a sticky error notification while the session survives -- + # it fires even though this effect destroyed itself above. + # + # A session-owned widget's comm can no longer open, so stop its + # placeholder queuing sends. A process-global one's placeholder is + # still drained when another session opens it (open_shared_in()). + if not is_process_global(w) and isinstance(w.comm, OrphanedShinyComm): + w.comm.close() + raise NotifyException( + f"Failed to display {type(w).__name__}: {e}", + sanitize=session.app.sanitize_errors, + ) from e # If the widget initialized in a reactive _output_ context, then cleanup the widget # when the context gets invalidated. Use the render output's Context (captured by diff --git a/tests/unit/test_callback_errors.py b/tests/unit/test_callback_errors.py index 2cfc773b..721c2e22 100644 --- a/tests/unit/test_callback_errors.py +++ b/tests/unit/test_callback_errors.py @@ -2,7 +2,9 @@ Exceptions raised by widget callbacks while handling a browser message are caught by ipywidgets and logged to the `traitlets` logger, whose only handler is a NullHandler, so outside of IPython they would vanish. shinywidgets re-routes -them to its own logger so they show up in the server's console. +them to its own logger so they show up in the server's console, and shows the +user a sticky error notification (one per widget), since the error usually +means their output is blank. """ from __future__ import annotations @@ -16,7 +18,7 @@ from ipywidgets import Button, IntSlider from shiny import Session, reactive from shiny.session import session_context -from shinywidgets._comm import ShinyComm +from shinywidgets._comm import ClientMsgScope, ShinyComm, handling_client_msg def flush() -> None: @@ -208,3 +210,144 @@ def schedule(change: object) -> None: [record] = caplog.records assert record.name == "traitlets" assert record.levelno == logging.WARNING + + +def test_scope_keeps_error_messages_not_exceptions(widget_session: Session) -> None: + # Deferred work (asyncio tasks, loop.call_soon) shares the dispatch scope + # via its copied context, so an exception kept on it would keep its + # traceback's frames -- and through them the widget, its callbacks and + # session -- alive for the lifetime of that work, even after the widget + # is closed or replaced. + scopes: List[ClientMsgScope] = [] + + def boom(change: object) -> None: + scope = handling_client_msg.get() + assert scope is not None + scopes.append(scope) + raise RuntimeError("observer failed") + + with session_context(widget_session): + slider = IntSlider(value=1) + slider.observe(boom, names="value") + comm = open_comm(slider) + comm.handle_msg( + client_msg( + comm.comm_id, + {"method": "update", "state": {"value": 5}, "buffer_paths": []}, + ) + ) + + [scope] = scopes + assert scope.errors == ["observer failed"] + + +def capture_notifications( + session: Session, monkeypatch: pytest.MonkeyPatch +) -> List[Dict[str, Any]]: + sent: List[Dict[str, Any]] = [] + monkeypatch.setattr(session, "_send_message_sync", sent.append) + return sent + + +def failing_slider() -> ShinyComm: + def boom(change: object) -> None: + raise RuntimeError("observer failed") + + slider = IntSlider(value=1) + slider.observe(boom, names="value") + comm = open_comm(slider) + comm.handle_msg( + client_msg( + comm.comm_id, + {"method": "update", "state": {"value": 5}, "buffer_paths": []}, + ) + ) + return comm + + +def test_observer_error_notifies_browser( + widget_session: Session, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + sent = capture_notifications(widget_session, monkeypatch) + with session_context(widget_session), caplog.at_level(logging.WARNING): + failing_slider() + + # Still logged server-side (the #304 behavior) ... + [record] = caplog.records + assert record.name == "shinywidgets" + # ... and now also shown to the user as a sticky error notification. + [note] = [m for m in sent if "notification" in m] + payload = note["notification"]["message"] + assert payload["type"] == "error" + assert payload["duration"] is None + assert "observer failed" in payload["html"] + + +def test_repeat_errors_reuse_the_widgets_notification( + widget_session: Session, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + sent = capture_notifications(widget_session, monkeypatch) + with session_context(widget_session), caplog.at_level(logging.WARNING): + comm = failing_slider() + comm.handle_msg( + client_msg( + comm.comm_id, + {"method": "update", "state": {"value": 6}, "buffer_paths": []}, + ) + ) + other = failing_slider() + + # Every failure is logged ... + assert len(caplog.records) == 3 + # ... but the browser replaces a notification whose id it already shows, + # so an observer that fails on every message leaves one per widget. + ids = [m["notification"]["message"]["id"] for m in sent if "notification" in m] + assert ids == [ + f"shinywidgets-error-{comm.comm_id}", + f"shinywidgets-error-{comm.comm_id}", + f"shinywidgets-error-{other.comm_id}", + ] + + +def test_warning_without_exception_does_not_notify( + widget_session: Session, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + def warn(change: object) -> None: + logging.getLogger("traitlets").warning("heads up") + + sent = capture_notifications(widget_session, monkeypatch) + with session_context(widget_session), caplog.at_level(logging.WARNING): + slider = IntSlider(value=1) + slider.observe(warn, names="value") + comm = open_comm(slider) + comm.handle_msg( + client_msg( + comm.comm_id, + {"method": "update", "state": {"value": 5}, "buffer_paths": []}, + ) + ) + + assert "heads up" in caplog.text + assert [m for m in sent if "notification" in m] == [] + + +def test_observer_error_notification_sanitized_when_app_sanitizes( + widget_session: Session, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + widget_session.app.sanitize_errors = True + sent = capture_notifications(widget_session, monkeypatch) + with session_context(widget_session), caplog.at_level(logging.WARNING): + failing_slider() + + [note] = [m for m in sent if "notification" in m] + assert note["notification"]["message"]["html"] == ( + "An error has occurred. Check your logs or contact the app author for clarification." + ) diff --git a/tests/unit/test_widget_display.py b/tests/unit/test_widget_display.py index b5157f16..9b3030dd 100644 --- a/tests/unit/test_widget_display.py +++ b/tests/unit/test_widget_display.py @@ -2,14 +2,15 @@ import asyncio import json +from typing import Any, Dict, List import pytest -from ipywidgets import Widget +from ipywidgets import IntSlider, Widget from plotly.graph_objects import FigureWidget, Scatter from shiny import Session, reactive from shiny.session import session_context from shiny.session._session import AppSession -from shinywidgets._comm import ShinyComm +from shinywidgets._comm import OrphanedShinyComm, ShinyComm from shinywidgets._shinywidgets import SESSION_WIDGET_ID_MAP @@ -71,3 +72,58 @@ def _repr_mimebundle_(self, **kwargs: object) -> dict[str, object]: class CustomFigureWidget(FigureWidget): pass + + +SANITIZED_MSG = "An error has occurred. Check your logs or contact the app author for clarification." + + +class BrokenStateSlider(IntSlider): + def get_state(self, *args: Any, **kwargs: Any) -> Any: + raise RuntimeError("boom during get_state") + + +def test_comm_open_failure_notifies_user( + widget_session: Session, monkeypatch: pytest.MonkeyPatch +) -> None: + sent: List[Dict[str, Any]] = [] + monkeypatch.setattr(widget_session, "_send_message_sync", sent.append) + + async def unhandled(e: Exception) -> None: + raise AssertionError(f"session must survive a comm-open failure: {e}") + + monkeypatch.setattr(widget_session, "_unhandled_error", unhandled) + + with session_context(widget_session): + _ = BrokenStateSlider() + asyncio.get_event_loop().run_until_complete(reactive.flush()) + + [note] = [m for m in sent if "notification" in m] + payload = note["notification"]["message"] + assert payload["type"] == "error" + assert payload["duration"] is None + assert "boom during get_state" in payload["html"] + + +def test_comm_open_failure_stops_queuing_sends(widget_session: Session) -> None: + with session_context(widget_session): + w = BrokenStateSlider() + asyncio.get_event_loop().run_until_complete(reactive.flush()) + w.send({"x": 1}) + + assert isinstance(w.comm, OrphanedShinyComm) + assert w.comm.take_queued_sends() == [] + + +def test_comm_open_failure_notification_is_sanitized_with_app_flag( + widget_session: Session, monkeypatch: pytest.MonkeyPatch +) -> None: + widget_session.app.sanitize_errors = True + sent: List[Dict[str, Any]] = [] + monkeypatch.setattr(widget_session, "_send_message_sync", sent.append) + + with session_context(widget_session): + _ = BrokenStateSlider() + asyncio.get_event_loop().run_until_complete(reactive.flush()) + + [note] = [m for m in sent if "notification" in m] + assert note["notification"]["message"]["html"] == SANITIZED_MSG