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

[WIP] Use WSGI server #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[WIP] Use WSGI server #99

wants to merge 2 commits into from

Conversation

thaeli
Copy link
Contributor

@thaeli thaeli commented Dec 7, 2017

No description provided.

Copy link
Member

@kitsuta kitsuta left a comment

Choose a reason for hiding this comment

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

A bit over my head but it looks good to me!

Copy link
Contributor

@EliAndrewC EliAndrewC left a comment

Choose a reason for hiding this comment

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

Apologies for having missed this PR when it first dropped. I really love GMail's "important" / "not important" views but sometimes I'm a little too trusting that it didn't miss anything important and go for far too long without scrolling through and looking for things I need to check.

I'm unclear what the code in sideboard/server.py is accomplishing. We're already able to set the socket host and socket port (and I'm pretty sure the thread pool as well) without doing the unsubscribe / subscribe stuff. If there's something being accomplished by this then we should at least add some comments explaining why we're doing what we're doing there.

@@ -155,6 +156,19 @@ def check_authentication(cls):
cherrypy_config[setting] = value
cherrypy.config.update(cherrypy_config)

# Unsubscribe the default server
cherrypy.server.unsubscribe()
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably need to delve into the cherrypy source code to figure out why we're doing this. Regardless, it would be useful to add a comment here explaining why. You have several comments explaining what you're doing, but I find "why" comments to be a lot more useful. In particular, "Unsubscribe the default server" doesn't tell me anything about the purpose of this code or what it's actually accomplishing. I'm pretty sure that we were already able to set server.socket_host and such in our config file, so I'm unclear what this code is doing.

@EliAndrewC EliAndrewC changed the title Use WSGI server [WIP] Use WSGI server Jan 6, 2018
@EliAndrewC
Copy link
Contributor

Ok, so CherryPy has a poorly documented way to do this.

  • There's a cherrypy.tree object
  • ...which has an apps dictionary whose keys are the URL paths like '/uber'
  • ...and whose values are usually cherrypy.Application objects
  • ...which have a wsgiapp property
  • ...which has a pipeline list
  • ...which is a list of tuples of the names and classes of WSGI middleware.

Let's put that all together. First I'll define an example middleware:

from sideboard.lib import log

class TestMiddleware:
    def __init__(self, app):
        self.app = app

    def __call__(self, environ, start_response):
        log.error('TestMiddleware called with environ => {}', environ)
        return self.app(environ, start_response)

So let's say that we want to add that to the application which serves everything under /uber:

cherrypy.tree.apps['/uber'].wsgiapp.pipeline.append(('TestMiddleware', TestMiddleware))

Assuming you wanted to have a list of middleware that you automatically applied to all cherrypy applications which support it:

wsgi_middleware = [
    ('TestMiddleware', TestMiddleware)
]

def add_wsgi_middleware_to_cherrypy():
    for app in cherrypy.tree.apps.values():
        if hasattr(app, 'wsgiapp'):
            app.wsgiapp.pipeline.extend(wsgi_middleware)

add_wsgi_middleware_to_cherrypy()

I've tested this out for this test middleware, though I haven't tested any more advanced middlewares.

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.

3 participants