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

Don't shutdown on reload signal(SIGHUP) when using the reloader option #376

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

hendrikmuhs
Copy link
Contributor

Fixes an issue with the reload option: Missing reload handling causes granian with reload=True to shutdown instead of respawning workers.

Copy link
Member

@gi0baro gi0baro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a some code changes suggestions.

In addition to that though, can you provide an use-case for this?
On a purely practical stand point, I can't foresee a local development condition in which you might rely on SIGHUP to reload Granian.

granian/server.py Outdated Show resolved Hide resolved
granian/server.py Outdated Show resolved Hide resolved
@hendrikmuhs
Copy link
Contributor Author

In addition to that though, can you provide an use-case for this?
On a purely practical stand point, I can't foresee a local development condition in which you might rely on SIGHUP to reload Granian.

We use Granian for an app that provides access to data that is stored locally(search indices). We use SIGHUP to update data: 1. download updates, 2. reload workers.

@gi0baro
Copy link
Member

gi0baro commented Aug 6, 2024

We use Granian for an app that provides access to data that is stored locally(search indices). We use SIGHUP to update data: 1. download updates, 2. reload workers.

That's an use-case for SIGHUP, but not reload + SIGHUP combo. And if you're using --reload in production, you should really not.
Do you have an use-case for reload + SIGHUP?

@hendrikmuhs
Copy link
Contributor Author

hendrikmuhs commented Aug 6, 2024

We use Granian for an app that provides access to data that is stored locally(search indices). We use SIGHUP to update data: 1. download updates, 2. reload workers.

That's an use-case for SIGHUP, but not reload + SIGHUP combo.

When I am developing locally I am also reloading data and therefore use SIGHUP. Reloading data is not exclusively for production.

And if you're using --reload in production, you should really not.

We are not using --reload in production.

Do you have an use-case for reload + SIGHUP?

Yes, whether I am running granian with or without --reload it should work the same with 1 difference: --reload should trigger a worker restart on code changes. Right now it implicitly changes the behavior for signal handling, too. A SIGHUP without --reload triggers a worker restart, a SIGHUP with --reload triggers a shutdown.

Let me explain how I am using SIGHUP for data reloading:

I have an API - lets call it /update - if that API gets triggered 1 worker out of n workers executes that request. So it can only update itself, not the other workers. The solution is therefore to prepare the update(download) and afterwards signal the master process(SIGHUP). The master process gracefully reloads workers, by doing that every worker reloads data and hence updates itself.

I know you have plans for interworker communication. This would be great for my usecase: instead of reloading workers, I could send a message to every worker to make the update. On other side, the above works well in a graceful manner.

@gi0baro
Copy link
Member

gi0baro commented Aug 13, 2024

@hendrikmuhs makes sense, thank you for the detailed explanation.
Pushed just a minor refactor, gonna merge it in the near future.

@gi0baro gi0baro merged commit 88811b3 into emmett-framework:master Aug 13, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants