Repository navigation
fix: close the root container after all worker shutdown code - #42
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_diclosed the root container in aWORKER_SHUTDOWNhandler. taskiq runs those handlers in list order and then calls middlewareshutdown(), so handlers added aftersetup_diand every middlewareshutdown()gotContainerClosedError, andAPPfinalizers had already run.How taskiq 0.12.6 runs startup and shutdown
AsyncBroker.startup()runs the event handlers for the event in list order, thenstartup()on each middleware that overrides it, thenresult_backend.startup().shutdown()does the same with the shutdown event, middlewareshutdown()andresult_backend.shutdown().add_middlewares()callsset_broker()and appends.event_handlersandmiddlewaresare public attributes, and nothing else runs after the middleware loop that an integration can hook into.InMemoryBrokeroverrides both methods. It runs the client and worker event handlers and never calls middlewarestartup()orshutdown(). So a closing middleware alone, as the issue suggests, would leave the root open forever onInMemoryBroker, and on any other broker that does the same.Approach
setup_dimakes one_RootClosermiddleware per call.WORKER_STARTUP,_open_rootopens the root, appends the closer tobroker.middlewaresif it is not there yet, and moves_close_rootto the end of theWORKER_SHUTDOWNhandlers. Because the closer is added during the handler loop, the middleware loop that follows calls itsstartup(), which records that this broker runs middleware hooks.WORKER_SHUTDOWN,_close_rootchecks that flag. If middleware hooks run, it moves the closer to the end ofbroker.middlewares(the list is not being iterated yet) and the closer'sshutdown()closes the root after every other middleware. If they do not run, or startup never happened,_close_rootcloses the root itself. It is the last shutdown handler by then, so it still runs after handlers added aftersetup_di.shutdown()only closes when itsstartup()ran in the same cycle, so the root closes exactly once.Restarts: the closer is never added twice, the flag resets on each startup, and the handler move is remove plus append, so neither list grows.
manage_lifespan=Falseregisters nothing, as before.Raised taskiq floor to 0.11.8
Before 0.11.8,
AsyncBroker.shutdown()passed the bound method tomaybe_awaitable(middleware.shutdown)without calling it, so no middlewareshutdown()ever ran. The closer'sstartup()did run, so on those versions the root would never close. The floors job caught this on 0.11.0. Since middleware shutdown does not work there at all, the issue's middleware criterion cannot be met on those versions, so the floor is nowtaskiq>=0.11.8.Limits
WORKER_SHUTDOWNhandler or middlewareshutdown()that raises stopsbroker.shutdown(), and the root is not closed. That was already true before.shutdown()never runs, so only the handler ordering applies.startup()but notshutdown()would leave the root open. No taskiq broker in 0.12.6 does this.Changes
modern_di_taskiq/main.py:_RootCloserand the new open and close handlers.tests/test_shutdown_order.py: a stubAsyncBrokerwithWORKER_SHUTDOWNhandlers and middlewares registered before and aftersetup_di, a middleware added after startup, two cycles with no duplicate middleware,InMemoryBrokerhandlers added aftersetup_diover two cycles, shutdown without startup, andmanage_lifespan=Falseon both brokers. The first four failed on main withContainerClosedError. The issue's repro now prints['user:live', 'finalized']for all four cases.README.mdandCONTEXT.md: state when the root closes.pyproject.toml:taskiq>=0.11.8,<0.13.Closes #41