sadpandajoe commented on code in PR #45478:
URL: https://github.com/apache/superset/pull/45478#discussion_r4406655442
##########
superset/tasks/manager.py:
##########
@@ -58,6 +58,33 @@ def wrapped() -> T:
return wrapped
+def _release_session_on_error(fn: Callable[[], T]) -> Callable[[], T]:
+ """Wrap ``fn`` so a failure rolls back and discards the scoped DB session.
+
+ When the listener thread is wrapped in a long-lived context (see
+ ``BACKGROUND_THREAD_CONTEXT_FACTORY``), no per-poll app context is pushed,
so
+ the usual teardown never runs. Without this, a failed query could leave the
+ session in a failed transaction and every later poll would fail too.
+ """
+
+ def wrapped() -> T:
+ try:
+ return fn()
+ except Exception:
Review Comment:
Cleanup only runs when `fn()` raises, so a poll that succeeds leaves the
scoped session's transaction open. With the factory's context wrapping the
whole listener thread, `_in_app_context` never pushes or pops a per-poll
context, so Flask-SQLAlchemy's teardown never releases it. Each running task
then holds one metastore connection for its whole lifetime: with `pool_size=1,
max_overflow=0`, one successful `check()` makes the next main-thread query
raise a pool `TimeoutError`, whereas it succeeds without the factory. Under
REPEATABLE READ (e.g. a MariaDB URI or an explicit `isolation_level`) that open
transaction also keeps the first poll's snapshot, so a later ABORTING status is
never seen. Could the session be removed after every poll (a `finally` around
`fn()`), and `test_successful_poll_leaves_session_alone` flipped to assert that?
##########
tests/unit_tests/coordination/test_service.py:
##########
@@ -456,3 +457,64 @@ 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:
+ checked_inside.append(events == ["enter"])
+ return True
+
+ fired = threading.Event()
+ listener = CoordinationService.listen_for_signal(
+ "ch", check=check, on_signal=fired.set, poll_interval=0.01
+ )
+ assert fired.wait(timeout=2.0) is True
+ listener.stop()
+ listener._thread.join(timeout=2.0)
+ finally:
+ current_app.config.pop("BACKGROUND_THREAD_CONTEXT_FACTORY", None)
+
+ assert factory_threads == [caller]
+ assert checked_inside == [True]
+ assert events == ["enter", "exit"]
+
+
+def test_listen_without_app_context_does_not_require_one(
+ mocker: MockerFixture,
+) -> None:
+ """Callers outside an app context get the no-op context, not a
RuntimeError."""
+ mocker.patch("superset.coordination.base.has_app_context",
return_value=False)
Review Comment:
`has_app_context` is patched, but the autouse `app_context` fixture in
`tests/unit_tests/conftest.py` keeps a real Flask context pushed, so
`current_app` still resolves. If the `has_app_context()` guard in
`_capture_background_context` were deleted, this test would still pass, even
though a genuinely context-free caller would hit `RuntimeError`. Could this run
`listen_for_signal` from a thread with no app context (asserting
`flask.has_app_context()` is false there) and check the callback still fires?
--
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]
