Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adds before_shutdown/after_startup lifespan hooks #3598

Closed
wants to merge 2 commits into from

Conversation

cofin
Copy link
Member

@cofin cofin commented Jun 25, 2024

Description

Closes: #3597

Closes

@github-actions github-actions bot added area/dependencies This PR involves changes to the dependencies area/docs This PR involves changes to the documentation labels Jun 25, 2024
@JacobCoffee
Copy link
Member

am I thinking of something else or did we have these before?

@provinzkraut
Copy link
Member

provinzkraut commented Jun 27, 2024

@JacobCoffee you're right, see #1663.

@cofin I'm not sure if this is a good solution or pattern we should support / promote.

This would effectively be the same then as adding something to the front of on_shutdown.

If the goal is to provide a way to say "run this before all on_shutdown hooks", it would just move the problem further down the line, because then, how do you say "run this before all the other before_shutdown hooks"?

IMO having hooks that depend on other hooks is an anti pattern. If your hooks involve state, you should use the lifespan context manager. And if they don't involve state, then the order they're called in shouldn't matter. If the order they are called in does matter, combine them together in a single hook.

(same applies to the startup hooks as well)


If we do want to support giving priorities to some hooks, then we should implement a priority configuration

@cofin
Copy link
Member Author

cofin commented Jun 30, 2024

@JacobCoffee you're right, see #1663.

@cofin I'm not sure if this is a good solution or pattern we should support / promote.

This would effectively be the same then as adding something to the front of on_shutdown.

If the goal is to provide a way to say "run this before all on_shutdown hooks", it would just move the problem further down the line, because then, how do you say "run this before all the other before_shutdown hooks"?

IMO having hooks that depend on other hooks is an anti pattern. If your hooks involve state, you should use the lifespan context manager. And if they don't involve state, then the order they're called in shouldn't matter. If the order they are called in does matter, combine them together in a single hook.

(same applies to the startup hooks as well)

If we do want to support giving priorities to some hooks, then we should implement a priority configuration

The specific need was after_startup. I addd in before_shutdown since it seemed to make logical sense at the time. I don't think we really need the before_shutdown.

While implementing these changes, I remembered us having this support previously, but I don't remember exactly why we removed though.

I do think there's a valid case for having a set of actions that can run after specific startup sequences have loaded, so maybe, we should think about a priority/ordering mechanism?

@provinzkraut
Copy link
Member

I do think there's a valid case for having a set of actions that can run after specific startup sequences have loaded, so maybe, we should think about a priority/ordering mechanism?

Definitely. IMO implementing priorities would be the way to go about this.

@cofin
Copy link
Member Author

cofin commented Aug 20, 2024

Closing for now.

@cofin cofin closed this Aug 20, 2024
auto-merge was automatically disabled August 20, 2024 20:16

Pull request was closed

@provinzkraut provinzkraut deleted the after-startup-before-shutdown branch September 14, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies This PR involves changes to the dependencies area/docs This PR involves changes to the documentation area/testing pr/internal size: small type/feat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Lifespan hook priorities
3 participants