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

Accomodate routing requests #311

Open
bluetech opened this issue Nov 18, 2017 · 6 comments
Open

Accomodate routing requests #311

bluetech opened this issue Nov 18, 2017 · 6 comments

Comments

@bluetech
Copy link
Contributor

Consider a websocket server with the following example URL structure:

  • /echo - First WS endpoint.
  • /chat/<room: string> - Second WS endpoint.
  • /health - Dumb HTTP endpoint
  • Any other URL -> 404.

If we only had the first two, it may be handled easily enough in the handler, with something like this:

ROUTES = (
    (r'^/echo$', echo_handler),
    (r'^/chat/([a-z]+)$', chat_handler),
)

async def ws_handler(websocket, path):
    for (pattern, handler) in ROUTES:
        match = re.match(pattern, path)
        if match:
            return await handler(websocket, *match.groups())
    assert False

To handle the /health endpoint, we can override websockets.WebSocketServerProtocol.process_request.

To handle the 404 requirement, it seems like it should also be handled in process_request, by checking ROUTES and failing if there's no match.

This is a little messy. Is there a better way to do this?


Some unsolicited thoughts:

In general I like that the library is focused and does not try to handle routing or be a full HTTP server.

There are 2 approaches I can see a library like this take:

  • The library has control and calls into user code. This is how Torando does it for example -- you define an Application object with routes, and Tornado parses the URL and calls the user's HTTP or WS handler.

  • The user has control and calls into the library. In the extreme this is how a library like wsproto works, but mostly what I mean is something like this:

ROUTES = (
    (r'$/health$', health_handler),
    (r'^/echo$', echo_handler),
    (r'^/chat/([a-z]+)$', chat_handler),
)
async for http_protocol in http_server:
    for route in ROUTES:
         if route matches the http request:
              spawn handler(http_protocol, *match.groups())
     spawn 404 handler(http_protocol)

async def health_handler(http_protocol):
    http_protocol.send(HTTPStatus.OK, b'OK')

async def chat_handler(http_protocol, room):
    ws_protocol = upgrade(http_protocol)
    async for message in ws_protocol:
         await ws_protocol.send('foo')

...

The current API does a little bit of both, so it works great for the intended use case, but becomes harder when straying a little.

@aaugustin
Copy link
Member

aaugustin commented Nov 20, 2017

That's a very good point.

The current design doesn't make it easy to implement this because it separates strongly the opening handshake (where you could return a 404) from the data transfer phase (the actual websockets protocol).

I think there's room for a higher level API than calling serve, possibly with a custom WebSocketServerProtocol. In addition to websockets.serve(handler, host, port) we could provide websockets.route(routes, host, port).

We could also add that to serve itself and dispatch on the type of the first argument... My gut feeling is that it would muddy the fact that route brings in an URL router and thus has a much larger scope.

There's also the question of designing the URL router. We could simply copy https://github.com/django/deps/blob/master/final/0201-simplified-routing-syntax.rst or the Flask URL router (which inspired DEP 201).

@aaugustin
Copy link
Member

aaugustin commented Nov 20, 2017

The URL router should be swappable anyway, similar to create_protocol=WebSocketServerProtocol for swapping the protocol class.

@cjerdonek
Copy link
Collaborator

Just a quick note in response to the original post:

This is a little messy. Is there a better way to do this?

I'm actually somewhat okay with how routing can be handled currently, though I agree it's not easy to "discover" how to best handle it.

Regarding the routing and 404 handling being split between WebSocketServerProtocol.process_request() and ws_handler(), here is a way to have all of the handling be done in process_request() to avoid this split:

The process_request() function can parse the URL, and if the URL should be handled as a websocket, process_request() can save the function to do the handling as an attribute of the WebSocketServerProtocol instance self. Then, ws_handler(websocket) can access this function as an attribute of websocket so that ws_handler() doesn't need to parse the URL again, etc.

@aaugustin
Copy link
Member

If we get a solid integration with sanic (cf. sanic-org/sanic#1214) we could point to sanic rather than write our own router.

Or we could write our own router because that's fun :-) and because I prefer centralizing the declaration of routes (like Django, unlike Flask).

@Tijani-Dia
Copy link

Tijani-Dia commented Nov 22, 2021

Hey all, I've made a small package for URL routing with this library. You can check it out here yrouter-websockets.

I'm hoping that it will be useful to some folks here and I'll be happy to receive any feedback.

@aaugustin
Copy link
Member

Another third-party solution courtesy of @uranusjr: https://github.com/uranusjr/websockets-routes .

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

No branches or pull requests

4 participants