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

Merged
merged 4 commits into from Mar 18, 2015

Conversation

Projects
None yet
5 participants
@wong2
Contributor

wong2 commented Mar 11, 2015

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.

@wong2

This comment has been minimized.

Show comment
Hide comment
@wong2

wong2 Mar 12, 2015

Contributor

@benoitc plz review

Contributor

wong2 commented Mar 12, 2015

@benoitc plz review

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Mar 12, 2015

Collaborator

Couldn't this be handled in https://github.com/benoitc/gunicorn/blob/master/gunicorn/workers/base.py#L89 or even in the Reloader class?

Collaborator

berkerpeksag commented Mar 12, 2015

Couldn't this be handled in https://github.com/benoitc/gunicorn/blob/master/gunicorn/workers/base.py#L89 or even in the Reloader class?

Show outdated Hide outdated gunicorn/util.py
("Content-Type", "text/plain"),
("Content-Length", str(len(msg)))
])
return iter([msg])

This comment has been minimized.

@berkerpeksag

berkerpeksag Mar 12, 2015

Collaborator

Why do you need to add iter() here? If I remember correctly, [msg] is enough in the WSGI spec.

@berkerpeksag

berkerpeksag Mar 12, 2015

Collaborator

Why do you need to add iter() here? If I remember correctly, [msg] is enough in the WSGI spec.

This comment has been minimized.

@wong2

wong2 Mar 12, 2015

Contributor

oh I copied it from the example on http://gunicorn.org/ , and you're right, [msg] is enough

@wong2

wong2 Mar 12, 2015

Contributor

oh I copied it from the example on http://gunicorn.org/ , and you're right, [msg] is enough

This comment has been minimized.

@wong2

wong2 Mar 14, 2015

Contributor

@berkerpeksag I've removed the iter() here

@wong2

wong2 Mar 14, 2015

Contributor

@berkerpeksag I've removed the iter() here

@wong2

This comment has been minimized.

Show comment
Hide comment
@wong2

wong2 Mar 12, 2015

Contributor

@berkerpeksag I don't think this can be handled in Reloader or reloader callback, since the SyntaxError is happened when the new worker is starting, while Reloader or reloader callback are in charge of terminating the old worker.

Contributor

wong2 commented Mar 12, 2015

@berkerpeksag I don't think this can be handled in Reloader or reloader callback, since the SyntaxError is happened when the new worker is starting, while Reloader or reloader callback are in charge of terminating the old worker.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Mar 15, 2015

Collaborator

This looks pretty good to me.

Collaborator

tilgovi commented Mar 15, 2015

This looks pretty good to me.

@wong2

This comment has been minimized.

Show comment
Hide comment
@wong2

wong2 Mar 16, 2015

Contributor

@tilgovi what else should I do before this can be merged?

Contributor

wong2 commented Mar 16, 2015

@tilgovi what else should I do before this can be merged?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Mar 16, 2015

Collaborator

I think it seems fine, I just want to give others a chance to look at it.

Collaborator

tilgovi commented Mar 16, 2015

I think it seems fine, I just want to give others a chance to look at it.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Mar 16, 2015

Owner

The patch works for me. But I would do the following changes:

  • only display the traceback in debug mode so people using the reloader for another purpose won't have it displayed
  • make the error template configurable related to #993

Thoughts?

Owner

benoitc commented Mar 16, 2015

The patch works for me. But I would do the following changes:

  • only display the traceback in debug mode so people using the reloader for another purpose won't have it displayed
  • make the error template configurable related to #993

Thoughts?

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Mar 16, 2015

Collaborator

Perhaps it would be good to make the try-except logic a method. This way, people can customize make_fail_app etc.

Collaborator

berkerpeksag commented Mar 16, 2015

Perhaps it would be good to make the try-except logic a method. This way, people can customize make_fail_app etc.

Show outdated Hide outdated gunicorn/workers/base.py
try:
self.wsgi = self.app.wsgi()
except SyntaxError as e:
if not self.reloader:

This comment has been minimized.

@berkerpeksag

berkerpeksag Mar 16, 2015

Collaborator

I'd use self.cfg.reload

@berkerpeksag

berkerpeksag Mar 16, 2015

Collaborator

I'd use self.cfg.reload

This comment has been minimized.

@tilgovi

tilgovi Mar 16, 2015

Collaborator

+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.

@tilgovi

tilgovi Mar 16, 2015

Collaborator

+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

This comment has been minimized.

Show comment
Hide comment
@wong2

wong2 Mar 18, 2015

Contributor

@benoitc is there still a debug mode in gunicorn? I can't find it in the doc

Contributor

wong2 commented Mar 18, 2015

@benoitc is there still a debug mode in gunicorn? I can't find it in the doc

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Mar 18, 2015

Owner

@wong2 no only debug log level (which provides the same informations) and spew setting.

Owner

benoitc commented Mar 18, 2015

@wong2 no only debug log level (which provides the same informations) and spew setting.

@wong2

This comment has been minimized.

Show comment
Hide comment
@wong2

wong2 Mar 18, 2015

Contributor

I've moved the try-catch codes to a new method load_wsgi so it's more customizable

Contributor

wong2 commented Mar 18, 2015

I've moved the try-catch codes to a new method load_wsgi so it's more customizable

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Mar 18, 2015

Collaborator

Looks great.

Collaborator

tilgovi commented Mar 18, 2015

Looks great.

benoitc added a commit that referenced this pull request Mar 18, 2015

Merge pull request #994 from wong2/reloader
reloader should survive SyntaxError

@benoitc benoitc merged commit 83be046 into benoitc:master Mar 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Mar 18, 2015

Collaborator

Thanks @wong2 :)

Collaborator

berkerpeksag commented Mar 18, 2015

Thanks @wong2 :)

@georgexsh

This comment has been minimized.

Show comment
Hide comment
@georgexsh

georgexsh Mar 19, 2015

Contributor

🍺

Contributor

georgexsh commented Mar 19, 2015

🍺

@wong2 wong2 deleted the wong2:reloader branch Jul 1, 2015

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