sadpandajoe commented on code in PR #45478:
URL: https://github.com/apache/superset/pull/45478#discussion_r4317923874
##########
tests/unit_tests/coordination/test_service.py:
##########
@@ -456,3 +457,48 @@ def blocking_xread(*_args: object, **_kwargs: object) ->
list[object]:
call.args[:2] == ("ch", {"m": "__wake__"})
for call in backend.xadd.call_args_list
)
+
+
+def test_listen_runs_inside_configured_background_context(
+ app_context: None, mocker: MockerFixture
+) -> None:
+ """The factory runs on the caller thread; its context wraps the listener
thread."""
+ from flask import current_app
+
+ mocker.patch.object(CoordinationService, "get_backend", return_value=None)
+ caller = threading.current_thread()
+ events: list[str] = []
+ factory_threads: list[threading.Thread] = []
+
+ class _Ctx:
+ def __enter__(self) -> None:
+ events.append("enter")
+
+ def __exit__(self, *exc: object) -> None:
+ events.append("exit")
+
+ def factory() -> Callable[[], _Ctx]:
+ factory_threads.append(threading.current_thread())
+ return _Ctx
+
+ current_app.config["BACKGROUND_THREAD_CONTEXT_FACTORY"] = factory
+ try:
+ checked_inside: list[bool] = []
+
+ def check() -> bool:
Review Comment:
Since `check()` returns `True` on its first call, the loop exits via signal
delivery after a single poll, so this test cannot tell "entered once for the
thread's lifetime" from "entered around each poll" - a regression that wraps
each `check()` call in the context would still produce `["enter", "exit"]`, and
the stop-driven exit path is never exercised. Could you make `check` return
`False` for a few polls (gated by events) and assert a single `enter`, no
`exit` before `listener.stop()`, then exactly one `exit` once the thread has
joined?
##########
superset/coordination/base.py:
##########
@@ -497,9 +498,16 @@ def listen_for_signal(
:param name: optional thread name suffix for logging.
"""
stop_event = threading.Event()
+ enter_context = cls._capture_background_context()
+
+ def run() -> None:
+ with enter_context():
+ cls._run_listen_loop(
+ channel, check, on_signal, stop_event, poll_interval
+ )
Review Comment:
Agreed—`_in_app_context` only pushes (and tears down) its own context when
none is active, so once the factory's context wraps `_run_listen_loop` the
per-poll teardown never runs. A metastore error inside `check()` then leaves
the thread's single session un-rolled-back, and later polls can keep raising
(e.g. `PendingRollbackError`) so the abort is never delivered; the session also
stays open for the task's lifetime. Should the loop reset the session
(rollback/remove) after each failed or completed poll when a factory context is
active?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
