Repository navigation
fix: close the root container after shutdown receivers connected after setup_di - #48
Merged
Merged
Conversation
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.
setup_diconnected its close receivers toworker_shutdownandworker_process_shutdownwhen it was called. Celery calls receivers in connection order, so a shutdown receiver connected aftersetup_digotContainerClosedError, andAPPfinalizers had already run.How Celery 5.6.3 dispatches signals
Signal.receiversis a list of(lookup_key, receiver)pairs.connectappends unless the key is already present, andsendcalls the receivers in list order (celery/utils/dispatch/signal.py,_connect_signaland_live_receivers). Withoutdispatch_uidthe key is(id(receiver), id(sender)), sodisconnect(receiver)followed byconnect(receiver, weak=False)moves a receiver to the end without duplicating it.senditerates a copy built by_live_receivers, so changing another signal's list during a send is safe.Approach
The
worker_initreceiver moves_close_for_workerto the end ofworker_shutdown, and theworker_process_initreceiver moves_close_for_processto the end ofworker_process_shutdown. A receiver connected before the worker starts, which is where user code connects them, now runs before the close. Each restart is a disconnect and a connect of the same function, so the receiver count stays the same. Prefork and spawn children runworker_process_initin the child, so the move happens there too. Nothing else changed: the ownership check andmanage_lifespan=Falsework as before.Warm shutdown with the threads and solo pools
Running tasks finish before
worker_shutdownfires, so nothing runs after the close.state.should_stop(celery/apps/worker.py,_shutdown_handler). The consumer loop raisesWorkerShutdownfromstate.maybe_shutdown()between events (celery/worker/loops.py,celery/worker/state.py). That happens outside the signal handler, sosignal_safe = Falseon the threads pool does not skip the shutdown.WorkController.startcatches it asSystemExitand callsstop(), which runs_shutdown(warm=True)(stopping the bootsteps in reverse order, the pool included) and only then_send_worker_shutdown()(celery/worker/worker.py).on_stopcallsThreadPoolExecutor.shutdown(), which waits for running and queued work (celery/concurrency/thread.py). The solo pool runs tasks inline in the loop, so a running task finishes beforemaybe_shutdown()gets a chance to raise.I checked this with a real
celery workeron both pools, using a memory broker and a 2 second task that resolves anAPPdependency, with SIGTERM sent during the task. The order wastask:start,task:end, userworker_shutdownreceiver (live),finalized. On main the same script printedfinalizedbefore the user receiver'sContainerClosedError.Limits
worker_initreceiver that runs after ours, still runs after the close.WorkController.terminate(), which never sendsworker_shutdown, so the root is not closed there. That was already true before this change.Changes
modern_di_celery/main.py: the init receivers move their close receiver to the end of its shutdown signal.tests/test_shutdown_order.py: for both signal pairs, a shutdown receiver connected aftersetup_diresolves anAPPdependency before the finalizer runs, two restart cycles leave the receiver counts unchanged and finalize once per cycle, andmanage_lifespan=Falseneither opens nor closes the root. The first test failed on main withContainerClosedError. The issue's repro now prints['user:live', 'finalized']in all four cases.README.md: say when the root closes.Closes #47