fix(examples): don't await state.startup() in the mount example — it deadlocks - #76
Open
Ang-dot wants to merge 1 commit into
Open
fix(examples): don't await state.startup() in the mount example — it deadlocks#76Ang-dot wants to merge 1 commit into
Ang-dot wants to merge 1 commit into
Conversation
service_mount.py's parent lifespan awaited erc8183_app.state.startup(). state.startup is a sync lambda returning the funded-job poll loop's asyncio.Task, and that loop only exits on shutdown - so awaiting it never returns. The lifespan never reached `yield`, so uvicorn started and then served nothing. The example was unrunnable as written. Fix is to call it without awaiting, which is what the returned Task is for. Also adds tests/test_mount_startup.py, pinning the three properties that made this a bug: startup is a sync callable, it returns a still-running Task, and awaiting that Task does not return. service_mount.py had no test coverage at all, which is why this shipped. Verified with fastapi 0.128.8 on py3.11: 9 passed before, 12 after.
Pull Request ReviewThis PR fixes a FastAPI mount-example startup deadlock by invoking the synchronous Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
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.
Summary
python/examples/agent-server/src/service_mount.pyis unrunnable as written. Its parent lifespan awaitserc8183_app.state.startup():state.startupis assigned aterc8183_server.py:470as:That is a sync lambda returning the poll loop's
asyncio.Task._funded_poll_loopiswhile True, exiting only whenstop_eventis set at shutdown. Soawait startup()awaits a Task that never completes: the lifespan never reachesyield, and uvicorn comes up without ever serving a request.Fix
Call it without awaiting — returning the Task is the point.
Plus a comment explaining why it must not be awaited, so it doesn't get "corrected" back.
Tests
service_mount.pyhad no test coverage, which is why this shipped. Addedtests/test_mount_startup.pypinning the three properties that made it a bug:test_startup_is_a_sync_callableawaitapplies to the return valuetest_startup_returns_a_still_running_taskasyncio.Taskthat is not donetest_awaiting_startup_never_returnsUses the same
monkeypatch(create_erc8183_state)stub convention astest_routes_poll.py, so no chain, wallet, or network access.Verified with
fastapi 0.128.8on Python 3.11:Notes for the maintainer
Two related things I did not change, since they are design calls rather than bugs:
state.startupis only assigned whenon_jobis passed (erc8183_server.py:469-470). Calling it on an app built withouton_jobraisesAttributeErrorrather than being a no-op.async def startup()that spawns and returns immediately would make the correct usage the natural one.The same deadlock existed in the published docs (
docs.bnbchain.orgSDK quickstart, Option 2) and is fixed separately in bnb-chain.github.io#882.