mikebridge commented on code in PR #45364:
URL: https://github.com/apache/superset/pull/45364#discussion_r4289661227
##########
superset/initialization/__init__.py:
##########
@@ -151,9 +151,21 @@ def configure_celery(self) -> None:
# setup properly
task_base = celery_app.Task
+ # ``celery_app`` is a process-wide singleton, so a second
initialization
+ # in the same process (the unit suite builds one app per test module)
+ # finds the wrapper installed by the previous one. Wrap the original
+ # base again instead of wrapping the wrapper: a chain of wrappers keeps
+ # every app ever initialized reachable through the task class
+ # hierarchy, and the suite's memory then grows with every module. Only
+ # this method installs the marked wrapper, so one level is unwrapped.
+ if "_superset_app_context_task" in vars(task_base):
+ task_base = task_base._superset_task_base
Review Comment:
Verified, and addressed in 0ba612656b (fix in b3ec172817, annotations in
0ba612656b).
Celery builds a task's class on first use, subclassing whatever
`celery_app.Task` is at that moment, and keeps that class in the task registry
for the rest of the process. The wrapper at this line captured its app in a
closure, so a task first used under an earlier app kept that app alive and
would push its context when called with no app context active. Probe (build app
#1, register and materialize a task, build app #2, call the task from no
context):
| Tree | Task built under #1 pushes | App #1 reachable through task classes
| Apps pinned by the 17 registered tasks |
|---|---|---|---|
| `master` | app #1 | yes, and every later task pins it too through the
chained wrappers | #1 by 17, #2 by 1 |
| previous head `f0fec1f3f7` | app #1 | yes, via the wrapper's `__call__`
closure | #1 by 16, #2 by 1 |
| `0ba612656b` | app #2 (current) | no, collected after `gc.collect()` |
none |
So it was not a regression against `master`, and it is not reachable in
production, which initializes once; inside an active app context the
`has_app_context()` short-circuit also runs the task under that context on
every tree. For the unit suite it did matter: each module that first used a
task pinned its app (4 of 8 apps alive at the end of a task-heavy 8-module
subset).
The wrapper now resolves the app from a module-level reference when the task
runs, and is installed once;
`test_task_registered_under_a_previous_app_runs_under_the_current_app` fails on
the previous head and passes now.
Generated by Claude (AI) on behalf of @mikebridge.
--
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]
