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

Graceful reload (HUP) is broken #25

Closed
MarSoft opened this issue Jan 14, 2016 · 5 comments
Closed

Graceful reload (HUP) is broken #25

MarSoft opened this issue Jan 14, 2016 · 5 comments

Comments

@MarSoft
Copy link
Contributor

MarSoft commented Jan 14, 2016

Seems that graceful reloading on SIGHUP (which is the feature of Gunicorn) is broken in Muffin.
For now I found two problems:

  1. Muffin expects its app object to be named app and is launched as muffin <module> run. It then loads app object from the module and sets that object to Gunicorn's app. Also it passes <module> to Gunicorn as app name.
    Now, when Gunicorn tries to reload on HUP, it parses app name and tries to separate it by ':' to determine app variable name; it then cannot find a ':' and defaults variable name to application, instead of app used by muffin: https://github.com/benoitc/gunicorn/blob/master/gunicorn/util.py#L352
    My current workaround for it is as follows:
    app = Muffin(...)
    application = app
  2. It probably sets bind address in some wrong way, or maybe Gunicorn tries to load it from config somehow (I didn't find out the details yet). As a result, when I first start muffin, it binds correctly; but after killing it with HUP, it rebinds to localhost:8000 with the following log:
[2016-01-14 15:55:40 +0300] [24358] [INFO] Starting gunicorn 19.3.0
[2016-01-14 15:55:40 +0300] [24358] [INFO] Listening at: http://127.0.0.1:5000 (24358)
[2016-01-14 15:55:40 +0300] [24358] [INFO] Using worker: muffin.worker.GunicornWorker
[2016-01-14 15:55:40 +0300] [24369] [INFO] Booting worker with pid: 24369
[2016-01-14 15:55:45 +0300] [24358] [INFO] Handling signal: hup
[2016-01-14 15:55:45 +0300] [24358] [INFO] Hang up: Master
[2016-01-14 15:55:45 +0300] [24358] [INFO] Listening at: [,<,g,u,n,i,c,o,r,n,.,s,o,c,k,.,T,C,P,S,o,c,k,e,t, ,o,b,j,e,c,t, ,a,t, ,0,x,7,f,1,d,9,8,b,f,1,1,6,0,>,]
[2016-01-14 15:55:45 +0300] [24477] [INFO] Booting worker with pid: 24477
[2016-01-14 15:55:45 +0300] [24369] [INFO] Stopping server: 24369, connections: 0
[2016-01-14 15:55:45 +0300] [24369] [INFO] Worker exiting (pid: 24369)

Line with [,<,g,u,n,i,... is result of a bug in Gunicorn - see benoitc/gunicorn#1181; actually it is a http://127.0.0.1:8000, which takes its origin from https://github.com/benoitc/gunicorn/blob/master/gunicorn/config.py#L498

For now I yet have no ideas on how to fix these problems.

@MarSoft
Copy link
Contributor Author

MarSoft commented Jan 14, 2016

For me all this looks like Manager.run command implementation (https://github.com/klen/muffin/blob/develop/muffin/manage.py#L89) is not correct: it sets configuration onto gunicorn app, but that configuration will not persist; probably we should leverage Gunicorn's command-line arguments parser (https://github.com/benoitc/gunicorn/blob/master/gunicorn/app/base.py#L140) or rather pass our configuration as "defaults" in our load_default_config implementation (https://github.com/klen/muffin/blob/develop/muffin/worker.py#L38): store these values in some object within a GunicornApp and load them with separate method which will be called both by Manager.run() and load_default_config():

from muffin.worker import GunicornApp

base_cfg = dict(
    bind = bind,
    daemon = daemon,
    <...>
)
gapp = GunicornApp(
    usage='...',
    base_config=base_cfg,
    config=self.app.cfg.CONFIG,
)
gapp.run()

and in load_default_config:

super().load_default_config()

del self.cfg.settings['paste']
del self.cfg.settings['django_settings']

# now load our defaults
for k,v in self._base_config.items():
    self.cfg.set(k, v)

self.cfg.settings['worker_class'].default = ...

@benoitc
Copy link

benoitc commented Jan 22, 2016

@MarSoft Thanks for the report I will look at it next week. Maybe @matrixise could also help there.

@benoitc
Copy link

benoitc commented Jan 22, 2016

@MarSoft a quick glance at the code let me think you're right. On HUP Gunicorn tries to reload every parameters from the conf by reloading it. But I will check.

@MarSoft
Copy link
Contributor Author

MarSoft commented Jan 23, 2016

This issue is also related to #19: Muffin's application class is loaded during initialization of Manager, so reloading will not work properly anyway. I think it can be fixed by rewriting manage.py so that run command will be handled specially, outside of Manager class and without loading Application subclass in main thread. Will try to prepare a pull request during this week.

@klen
Copy link
Owner

klen commented Jan 26, 2021

Doesn't actual for version > 0.45

@klen klen closed this as completed Jan 26, 2021
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

No branches or pull requests

3 participants