bpo-40700: make WSGIRequestHandler can use an easy way to replace the ServerHandler

[Zheaoli]

https://bugs.python.org/issue40700

https://bugs.python.org/issue40700

[FFY00]

I think we could use some checks to make sure server_handler is valid, instead of just giving somewhat obscure runtime errors when it is not.

Having a test would also be great :)

[remilapeyre]

That seems a good idea, can you add some tests and update the documentation for the new attribute?

[remilapeyre]

I don't think this check is appropriate, if someone duck-types BaseHandler, the RequestHandler should accept it restricting the user.

[FFY00]

Why would someone duck-type BaseHandler instead of just subclassing it. If there's a good reason then that is fine, otherwise we are just supporting calling this in yet another way, and that way requires us to implement a more complex verification routine.

[Zheaoli]

@remilapeyre I agree with @FFY00, we should make it as simple as we can

[zhangyangyu]

I don't think check a class variable inside instance constructor is a good idea. I think it's fine to omit it and use server_handler directly. Those who want to extend should ensure it's of correct type.

[FFY00]

It is a class variable, not instance variable. I wonder if the implementation would not be better just making it an instance variable and passing the argument in the constructor.

[Zheaoli]

Because the BaseHandler initial process is dependent on some special variable what is existed in the request. So I don't think to make it as an instance variable is a good idea

[Zheaoli]

I'll fix the docs

[FFY00]

Still, I don't see the problem on setting server_handler in the constructor. Am I missing some context here?

If you make it a class attribute, if you only want to change the server handler you still need to subclass WSGIRequestHandler.

[Zheaoli]

        handler = self.server_handler(
            self.rfile, self.wfile, self.get_stderr(), self.get_environ(),
            multithread=False,
        )

the BaseHandler initial process needs the request environment, here's the detail

    def get_environ(self):
        env = self.server.base_environ.copy()
        env['SERVER_PROTOCOL'] = self.request_version
        env['SERVER_SOFTWARE'] = self.server_version
        env['REQUEST_METHOD'] = self.command
        if '?' in self.path:
            path,query = self.path.split('?',1)
        else:
            path,query = self.path,''

        env['PATH_INFO'] = urllib.parse.unquote(path, 'iso-8859-1')
        env['QUERY_STRING'] = query

        host = self.address_string()
        if host != self.client_address[0]:
            env['REMOTE_HOST'] = host
        env['REMOTE_ADDR'] = self.client_address[0]

        if self.headers.get('content-type') is None:
            env['CONTENT_TYPE'] = self.headers.get_content_type()
        else:
            env['CONTENT_TYPE'] = self.headers['content-type']

        length = self.headers.get('content-length')
        if length:
            env['CONTENT_LENGTH'] = length

        for k, v in self.headers.items():
            k=k.replace('-','_').upper(); v=v.strip()
            if k in env:
                continue                    # skip content length, type,etc.
            if 'HTTP_'+k in env:
                env['HTTP_'+k] += ','+v     # comma-separate multiple headers
            else:
                env['HTTP_'+k] = v
        return env

Some of the environment message is only existed in the HTTP request, that means that we need make an instance of BaseHandler for every request income

[FFY00]

Sorry, I still fail to see a reason why it wouldn't be better to just have this:

class WSGIRequestHandler(BaseHTTPRequestHandler):
    ...
    def __init__(self, server_handler=ServerHandler, *args, **kwargs):
        self.server_handler = server_handler
        ...

get_environ is an instance method so it never gets called in a context where self.server_handler would not be set.

[Zheaoli]

Oh, I think I get your means, yes, this would be a good idea

[Zheaoli]

@FFY00 Oh No, it doesn't work actually. I have double-checked the code, here's the detail

The WSGIRequestHandler initial process is not controlled by the user, by the server_class

def make_server(
    host, port, app, server_class=WSGIServer, handler_class=WSGIRequestHandler
):
    """Create a new WSGI server listening on `host` and `port` for `app`"""
    server = server_class((host, port), handler_class)
    server.set_app(app)
    return server

The user always uses the ReqeustHandler as a param when they try to use the make_server, and let the server_class to control it when it can be made as an instance. So, if we change the constructor, I think it will make a big and broken change for the make_server function and server_class

[FFY00]

That makes sense 😄

[Zheaoli]

Thanks bro/mses, this discussion is very useful for
me!😀😀

[zhangyangyu]

I don't know we should expose this attribute or not. For me, the handle() method has already document the behaviour very well.

[cpython-cla-bot]

The following commit authors need to sign the Contributor License Agreement:

• me@manjusaka.me

Click the button to sign: