Skip to content

🐛 Ensure that app.include_router merges nested lifespans#9630

Merged
tiangolo merged 20 commits into
fastapi:masterfrom
Lancetnik:master
Aug 24, 2024
Merged

🐛 Ensure that app.include_router merges nested lifespans#9630
tiangolo merged 20 commits into
fastapi:masterfrom
Lancetnik:master

Conversation

@Lancetnik

@Lancetnik Lancetnik commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

I am working on my own Propan library and as a part of it I implemented a custom FastAPI router with the following behavior.

from fastapi import FastAPI
from propan.fastapi import RabbitRouter

router = RabbitRouter("amqp://guest:guest@localhost:5672")
app = FastAPI()
app.include_router(router)

To connect RabbitMQ at FastAPI startup I used router.on_startup events and all worked fine, but now it is deprecated. So I migrate to lifespan and now FastAPI doesn't run router lifespan at the code above.

I suppose it was missed cuz the original include_router method includes all nested on_startup and on_shutdown deprecated events here, but doesn't includes a router lifespan.

So, the following code doesn't work

from contextlib import asynccontextmanager
from fastapi import FastAPI, APIRouter

@asynccontextmanager
async def router_lifespan(app):
    print('router start')
    yield
    print('router shutdown')

router = APIRouter(lifespan=router_lifespan)
app = FastAPI()
app.include_router(router)

The current PR fixes the original framework miss.
All original test and lint.sh checks works fine.

@github-actions

github-actions Bot commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

📝 Docs preview for commit 860c851 at: https://647f7cdb633e285f9b5dace9--fastapi.netlify.app

@Lancetnik Lancetnik changed the title Fix: app.include_router doesn't nest lifespans Fix: app.include_router doesn't merge nested lifespans Jun 6, 2023
@github-actions

github-actions Bot commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

📝 Docs preview for commit 984e59f at: https://647f7f0b29b2f764bcaa3dd9--fastapi.netlify.app

@tiangolo

Copy link
Copy Markdown
Member

📝 Docs preview for commit 8f26ed5 at: https://6484cca7bf187a105b276a5e--fastapi.netlify.app

@Lancetnik

Copy link
Copy Markdown
Contributor Author

@tiangolo can you take a look here, please? It's a minor bugfix, but it's important for my project.

@Lancetnik Lancetnik changed the title Fix: app.include_router doesn't merge nested lifespans Bugfix: app.include_router doesn't merge nested lifespans Jun 16, 2023
@tiangolo

Copy link
Copy Markdown
Member

📝 Docs preview for commit f322c99 at: https://648c2ec07c8ac9541405829c--fastapi.netlify.app

@tiangolo

Copy link
Copy Markdown
Member

📝 Docs preview for commit 3e2fd98 at: https://648e16b538df29410a152328--fastapi.netlify.app

@Kludex

Kludex commented Jun 17, 2023

Copy link
Copy Markdown
Member

@tiangolo can you take a look here, please? It's a minor bugfix, but it's important for my project.

This is not minor, and ideally we'd like this to live in Starlette.

@Lancetnik

Copy link
Copy Markdown
Contributor Author

@Kludex maybe, but how we can move it to Starlette if include_router is the FastAPI-level method? Do you suggest move the method to Starlette?

@Lancetnik

Copy link
Copy Markdown
Contributor Author

This will require a lot of work in both FastAPI and Starlette, since none of the FastAPI methods uses super().add_route() from Starlette. Therefore, it is necessary to rewrite all the methods add_api_route, add_websocket_route, add_api_websocket_route to use the parent methods of Starlette. It will also require a consistent update of these two packages. It seems to me that this is too difficult way, but I will do it. Perhaps it makes sense to merge the current PR for now before we can make a more elegant solution.

@Lancetnik

Copy link
Copy Markdown
Contributor Author

@Kludex, so can you check a Starlette PR?

@Lancetnik

Copy link
Copy Markdown
Contributor Author

This is not minor, and ideally we'd like this to live in Starlette.

@Kludex this bug doesn't on Starlette side and not connected with this Issue or this Starlette PR or Issue.

It should be fixed in FastAPI cuz we have no Starlette calls inside include_router method. It doesn't use Starlette Host or Mount that can be nested at Starlette side, but copies all included router attributes to app.router directly. So, we should merge the incuded lifespan here too.

So can you review and approve this PR?

@Kludex

Kludex commented Jun 23, 2023

Copy link
Copy Markdown
Member

Starlette should support running the lifespan of other routers... It shouldn't be here.

@Lancetnik

Lancetnik commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

Starlette should support running the lifespan of other routers... It shouldn't be here.

But there no other routers. We have the original app.router and copy all routes of included router to it. What do you want to do with it?

@Kludex just take a look at FastAPI original code here

A FastAPI application works with an only one original router (if we didn't use app.mount to mount another FastAPI application). So how we can use Starlette lifespan resolving logic here?

@ovflw

ovflw commented Jul 12, 2024

Copy link
Copy Markdown

+1

1 similar comment
@Flosckow

Copy link
Copy Markdown

+1

@FraterCRC

Copy link
Copy Markdown

+1)

@ApostolFet

Copy link
Copy Markdown

+1

@iNerV

iNerV commented Jul 19, 2024

Copy link
Copy Markdown

Wait for fix

@bomzheg

bomzheg commented Jul 20, 2024

Copy link
Copy Markdown

+1

1 similar comment
@jekel

jekel commented Jul 22, 2024

Copy link
Copy Markdown

+1

@Tishka17

Copy link
Copy Markdown

@tiangolo can we have a minute of your time here?

@Tishka17

Tishka17 commented Aug 6, 2024

Copy link
Copy Markdown

I am sorry to annoy you, but it is 14 months since PR is opened. Can we, as a community, have a decision or plan?

@Lancetnik

Copy link
Copy Markdown
Contributor Author

@svlandeg @estebanx64 @alejsdev Hello, team! Sorry for such annoyiment, but we are still waiting for any decision about this PR.

FastAPI now has an obvious bug / weaknesses. And we have the PR fixing it.

I know, that this thing should be fixed in the future according to Roadmap, but we are waiting year already for it. Have we any reason to not merge 10 lines of code if they fixing the problem already and we can remove them in the future as a part of Roamap feature scope?

@chessenjoyer17

Copy link
Copy Markdown

+1

3 similar comments
@Alkalit

Alkalit commented Aug 7, 2024

Copy link
Copy Markdown

+1

@Cub11k

Cub11k commented Aug 7, 2024

Copy link
Copy Markdown

+1

@chirizxc

chirizxc commented Aug 7, 2024

Copy link
Copy Markdown

+1

@prostomarkeloff

Copy link
Copy Markdown
Contributor

@dmitryleafall

Copy link
Copy Markdown

+1

@fastapi fastapi locked as spam and limited conversation to collaborators Aug 7, 2024
Comment thread fastapi/routing.py Outdated
@Kludex Kludex added p1 and removed p4 labels Aug 7, 2024
@svlandeg svlandeg changed the title Bugfix: app.include_router doesn't merge nested lifespans 🐛 Ensure that app.include_router merges nested lifespans Aug 23, 2024
@svlandeg svlandeg added the bug Something isn't working label Aug 23, 2024
@tiangolo

Copy link
Copy Markdown
Member

Thank you for your contribution @Lancetnik! 🍰

Thanks everyone for the input. ☕

Thanks a lot for the help, as always, @Kludex 🙏 🙇

This will be available in FastAPI version 0.112.2, released in the next few hours. 🎉


Thanks everyone for the patience. As this involved some internal changes, that means there's a bit of a higher chance it could break something else elsewhere (something we didn't expect and didn't account for), so I wanted to handle some other lower-impact issues and releases first.

Now I was finally able to sit a few hours dedicated just to this, read several parts of the ASGI spec again, ensure this would be the right way to handle things (e.g. what nested lifespan states should override which), etc.

Now, time to enjoy your new nested lifespans. 😎 ☕

@tiangolo tiangolo merged commit 3a4ac24 into fastapi:master Aug 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working p1

Projects

None yet

Development

Successfully merging this pull request may close these issues.