-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix recursive WorkController instantiation in DjangoWorkerFixup + AttributeError when pool_cls is a string #10045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix recursive WorkController instantiation in DjangoWorkerFixup + AttributeError when pool_cls is a string #10045
Conversation
- Remove worker parameter from DjangoWorkerFixup.__init__ - Set worker via on_worker_init callback instead - Add None check for worker.pool_cls to prevent AttributeError - Add regression test to prevent future recursion bugs Fixes recursive instantiation issue where DjangoWorkerFixup(app) would create WorkController(app), which in turn would create another DjangoWorkerFixup, leading to infinite recursion. The worker instance is now properly set via the on_worker_init signal callback, avoiding the circular dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical recursion bug in DjangoWorkerFixup.__init__ that caused infinite recursion during worker instantiation when using Django with Celery. The root cause was that DjangoWorkerFixup.__init__ was creating a new WorkController instance, which triggered the worker_init signal, leading to recursive DjangoWorkerFixup instantiation.
The solution removes the automatic WorkController instantiation from DjangoWorkerFixup.__init__ and instead sets the worker instance via the DjangoFixup.on_worker_init callback, breaking the circular dependency. Additionally, the PR adds a None check before accessing worker.pool_cls to prevent AttributeError when the worker hasn't been set yet.
Key Changes
- Removed the
workerparameter fromDjangoWorkerFixup.__init__signature - Added a class attribute
worker = Nonewith an explanatory comment - Added None check before accessing
worker.pool_cls.__module__to safely handle cases where worker isn't set
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| celery/fixups/django.py | Removed worker parameter from DjangoWorkerFixup.__init__, set worker = None as class attribute, and added None check in _close_database() to prevent AttributeError |
| t/unit/fixups/test_django.py | Updated fixup_context helper to set worker mock for all tests, simplified test_install, and added regression test test_no_recursive_worker_instantiation |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10045 +/- ##
==========================================
+ Coverage 88.41% 88.42% +0.01%
==========================================
Files 153 153
Lines 19363 19368 +5
Branches 2226 2227 +1
==========================================
+ Hits 17120 17127 +7
+ Misses 1943 1941 -2
Partials 300 300
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
auvipy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to add additional Integration tests for this?
|
Great fix! This correctly addresses the root cause of the recursion issue. Suggestions1. Similar patterns in other filesCopilot identified 2 other locations with similar
While your fix ensures 2. Integration test ideaFor the integration test that @auvipy requested, here's a possible approach based on existing patterns in import subprocess
import pytest
def test_django_fixup_no_recursion():
"""Test that Django fixup doesn't cause RecursionError on worker startup."""
# Create a minimal Django settings module for testing
result = subprocess.run(
["celery", "-A", "some_django_app", "worker", "--pool=solo", "-c", "1"],
capture_output=True,
timeout=10,
env={**os.environ, "DJANGO_SETTINGS_MODULE": "test_settings"}
)
output = result.stderr.decode('utf-8')
# Verify no RecursionError or AttributeError related to pool_cls
assert "RecursionError" not in output
assert "'str' object has no attribute '__module__'" not in outputThis would require a minimal Django project setup in the test fixtures. I closed my PR #10043 in favor of this one since it fixes the root cause properly. 👍 |
|
@maycuatroi1 thanks! Actually, when I was implementing the test case, I realized that pool_cls could still be a string depending on the pool, then I kept what you had done in your PR to properly check if it's a string. @auvipy I think I figured out why you were not able to reproduce the issue. When I tried to setup the integration test, I wasn't able to see the exceptions, even though they were happening (it was evident using debug). After hours trying to understand what was missing, I realized that a simple logging.info was making that exception visible. That's the principle of the integration test - checking the log output. That test case is supposed to break on v5.6.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
|
@bruunotrindade I can help fix the failing CI (unit tests on 3.10+) and add the defensive checks for other |
|
Also, I can test this fix on my staging environment for the issue #10042. |
auvipy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the unit test failure
Extends the fix from celery#10045 by adding defensive checks for when pool_cls is a string instead of a class in two additional locations: - celery/contrib/testing/worker.py: TestWorkController.__init__ - celery/worker/components.py: Beat.create This prevents AttributeError: 'str' object has no attribute '__module__' when pool_cls is passed as a string (e.g., 'prefork', 'gevent', 'eventlet'). Also adds unit tests to verify the defensive checks work correctly.
|
@auvipy I fixed the tests. I don't know why, but the issue was on tox.ini |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
…ule__ (#10048) * Fix recursive WorkController instantiation in DjangoWorkerFixup - Remove worker parameter from DjangoWorkerFixup.__init__ - Set worker via on_worker_init callback instead - Add None check for worker.pool_cls to prevent AttributeError - Add regression test to prevent future recursion bugs Fixes recursive instantiation issue where DjangoWorkerFixup(app) would create WorkController(app), which in turn would create another DjangoWorkerFixup, leading to infinite recursion. The worker instance is now properly set via the on_worker_init signal callback, avoiding the circular dependency. * fix: improve prefork detection and add integration test * fix: Copilot recommendations * Add defensive checks for pool_cls.__module__ in additional locations Extends the fix from #10045 by adding defensive checks for when pool_cls is a string instead of a class in two additional locations: - celery/contrib/testing/worker.py: TestWorkController.__init__ - celery/worker/components.py: Beat.create This prevents AttributeError: 'str' object has no attribute '__module__' when pool_cls is passed as a string (e.g., 'prefork', 'gevent', 'eventlet'). Also adds unit tests to verify the defensive checks work correctly. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add unit tests for TestWorkController with string pool_cls Add tests to verify defensive handling of string pool_cls in TestWorkController.__init__, addressing reviewer feedback. --------- Co-authored-by: Asif Saif Uddin {"Auvi":"অভি"} <auvipy@gmail.com>
|
@bruunotrindade I just wanted to say thank you so much for submitting this fix! It made a big difference. |
Description
This PR fixes a critical recursion bug in
DjangoWorkerFixup.__init__that causes infinite recursion during worker instantiation when using Django with Celery, and causes #10042Root cause
DjangoWorkerFixup.__init__was creating aWorkControllerinstance via theworkerorWorkController(app)pattern. This created a circular dependency where:WorkController.__init__triggers theworker_initsignalDjangoFixup.on_worker_initcreates/accessesDjangoWorkerFixupDjangoWorkerFixup.__init__tries to create anotherWorkControllerMore details of these comments:
#10023 (comment)
#10023 (comment)
Solution
Remove the automatic
WorkControllerinstantiation from__init__and instead set the worker instance via theon_worker_initcallback, breaking the circular dependency.Changes
django.pyworkerparameter fromDjangoWorkerFixup.__init__worker = Noneas a class attribute with an explanatory commentNonecheck before accessingworker.pool_clsto preventAttributeErrorDjangoFixup.on_worker_initcallbacktest_django.pyfixup_contexthelper to properly set worker mock for all teststest_no_recursive_worker_instantiationthat verifies noWorkControlleris created when it shouldn't beTesting
This test case is a way to prove that the recursion issue happens on v5.6.1:
Related Issues
#10023 #10023