Skip to content

fix: use lifespan context manager in Starlette and FastAPI - #368

Merged
cofin merged 10 commits into
mainfrom
fastapi-and-more-hash
Jan 26, 2025
Merged

fix: use lifespan context manager in Starlette and FastAPI#368
cofin merged 10 commits into
mainfrom
fastapi-and-more-hash

Conversation

@cofin

@cofin cofin commented Jan 25, 2025

Copy link
Copy Markdown
Member

Modifes the Starlette and FastAPI integrations to use the lifespan context manager instead of the startup`shutdown` hooks. If the application already has a lifespan set, it is wrapped so that both execute.

@cofin

cofin commented Jan 25, 2025

Copy link
Copy Markdown
Member Author

Closes #367

@cofin cofin linked an issue Jan 25, 2025 that may be closed by this pull request
4 tasks
@PokkaKiyo

PokkaKiyo commented Jan 26, 2025

Copy link
Copy Markdown
Contributor

@cofin If you don't mind me adding, the changes you made to AdvancedAlchemy seem like it would run each config's on_startup/on_shutdown twice if the user does not define a custom lifespan, but only once if the user does define lifespan.

That's probably fine for create_all and engine.dispose, but maybe the add_event_handler functions in the config classes should be removed if this isn't intended?

Also the outer shutdown doesn't run when hit with an exception 😅

@cofin

cofin commented Jan 26, 2025

Copy link
Copy Markdown
Member Author

@cofin If you don't mind me adding, the changes you made to AdvancedAlchemy seem like it would run each config's on_startup/on_shutdown twice if the user does not define a custom lifespan, but only once if the user does define lifespan.

That's probably fine for create_all and engine.dispose, but maybe the add_event_handler functions in the config classes should be removed if this isn't intended?

Also the outer shutdown doesn't run when hit with an exception 😅

Hah. Is it obvious I don't use FastAPI daily? :)

@cofin

cofin commented Jan 26, 2025

Copy link
Copy Markdown
Member Author

@cofin If you don't mind me adding, the changes you made to AdvancedAlchemy seem like it would run each config's on_startup/on_shutdown twice if the user does not define a custom lifespan, but only once if the user does define lifespan.

That's probably fine for create_all and engine.dispose, but maybe the add_event_handler functions in the config classes should be removed if this isn't intended?

Also the outer shutdown doesn't run when hit with an exception 😅

Your suggestions were spot on. I think I've finally addressed all of the issues on this one. Thanks again.

@cofin
cofin force-pushed the fastapi-and-more-hash branch from e0b10aa to eb00469 Compare January 26, 2025 18:28
@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

Documentation preview will be available shortly at https://litestar-org.github.io/advanced-alchemy-docs-preview/368

@cofin
cofin merged commit bb454da into main Jan 26, 2025
@cofin
cofin deleted the fastapi-and-more-hash branch January 26, 2025 18:57
@PokkaKiyo

Copy link
Copy Markdown
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: startup and shutdown events don't run in fastapi apps with lifespan

3 participants