-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
reloader should survive SyntaxError #994
Conversation
@benoitc plz review |
Couldn't this be handled in https://github.com/benoitc/gunicorn/blob/master/gunicorn/workers/base.py#L89 or even in the Reloader class? |
("Content-Type", "text/plain"), | ||
("Content-Length", str(len(msg))) | ||
]) | ||
return iter([msg]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to add iter()
here? If I remember correctly, [msg]
is enough in the WSGI spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I copied it from the example on http://gunicorn.org/ , and you're right, [msg]
is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@berkerpeksag I've removed the iter()
here
@berkerpeksag I don't think this can be handled in |
This looks pretty good to me. |
@tilgovi what else should I do before this can be merged? |
I think it seems fine, I just want to give others a chance to look at it. |
The patch works for me. But I would do the following changes:
Thoughts? |
Perhaps it would be good to make the try-except logic a method. This way, people can customize make_fail_app etc. |
try: | ||
self.wsgi = self.app.wsgi() | ||
except SyntaxError as e: | ||
if not self.reloader: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use self.cfg.reload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
On Mon, Mar 16, 2015, 16:26 Berker Peksag notifications@github.com wrote:
In gunicorn/workers/base.py
#994 (comment):@@ -116,7 +117,19 @@ def changed(fname):
self.init_signals()
self.wsgi = self.app.wsgi()
try:
self.wsgi = self.app.wsgi()
except SyntaxError as e:
if not self.reloader:
I'd use self.cfg.reload
—
Reply to this email directly or view it on GitHub
https://github.com/benoitc/gunicorn/pull/994/files#r26533913.
@wong2 no only |
I've moved the try-catch codes to a new method |
Looks great. |
reloader should survive SyntaxError
Thanks @wong2 :) |
🍺 |
This implements the feature requested in issue #964.
When there is
SyntaxError
raised while importing the app, provides a fallback app to display those errors instead of completely exit gunicorn.