Repository navigation
fix: keep the root container off app.conf so spawn children start - #42
Merged
Merged
Conversation
setup_di now registers the container in a weak registry keyed by app, so pickling the app no longer pickles the container. In a spawn child the worker_process_init handler binds its container to the unpickled current app when the main names match. Closes #41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #41
Problem
setup_distored the root container inapp.conf. Under the spawn start method (Windows, or forced), billiard pickles the app,conf.changesincluded, for every pool child.Containeris not picklable, so the prefork pool failed to start withcannot pickle '_thread._local' object. Fork, the default on macOS and Linux, never pickles the app, so it was unaffected.Change
setup_diregisters the container in a module-levelweakref.WeakKeyDictionarykeyed by the app object. Nothing DI-related goes intoapp.conf, sopickle.dumps(app)works.fetch_di_containerand@injectresolve thecurrent_appproxy with_get_current_object()before the lookup, since the proxy can't be weakly referenced.worker_process_inithandler opens its container as before, then binds it to the process's current app when that app'smainequals themainof the app it was set up for. A spawn child makes the unpickled app current and re-imports the task modules, which re-runssetup_dion a new app object. The binding connects the two.setdefault, so an app that already has a container keeps it. Under fork the current app is the module app, which already has its entry, so the bind does nothing there. It also stops a stale handler from an earliersetup_dion another app with the samemainfrom replacing a live container.Behavior change:
app.confno longer has amodern_di_containerkey. Code that read it directly instead of callingfetch_di_containerwill break.Tests
New
tests/test_spawn.pycovers a pickle round trip of a configured app,mainsurviving the round trip, a simulated spawn child (unpickled app as current,setup_dion a re-created app, thenworker_process_init), two apps with differentmainnames, and the no-overwrite rule. The existing tests pass unchanged.I didn't add a real-worker test. I ran one by hand on macOS with celery 5.6.3 and billiard 4.3.1, a filesystem broker, a file result backend and
FORKED_BY_MULTIPROCESSING=1:@injecttask ran inSpawnPoolWorker-1and returned.ForkPoolWorker-1.TypeError: cannot pickle '_thread._local' object.Docs
The README has a new "Prefork with spawn" note covering fork as the default, the
mainmatching, andFORKED_BY_MULTIPROCESSING=1. CONTEXT.md no longer says the root container lives onconf. The matching note for the Celery page in the modern-di docs will come in a separate follow-up PR in that repo.