New websockets

[ashleysommer]

Resolves #2000

@aaugustin @ahopkins
I've spent another couple of days on it, and its now passing all of the tests, including some other demo applications I've written to stress it out.

It certainly needs cleaning up, and could probably get some efficiency improvements in places too, but otherwise I'm pretty happy with it.

This branch also incorporates another change I've written, that splits the existing server.HttpProtocol out into a base SanicProtocol class, that contains just all of the connection-handling and transport-layer stuff, and HttpProtocol that is a subclass of SanicProtocol and adds the HTTP-specific handlers on top of that. I'd like the new WebSocketProtocol to subclass SanicProtocol but for now it still subclasses HttpProtocol.

[ahopkins]

Looks fantastic. I will pull the branch and review some more.

[aaugustin]

Looking at this from the perspective of websockets, I'm planning to review how you handle connection termination, normal or abnormal, because this is the area where I'm least confident that websockets gives you a nice API. Fortunately you're only doing the server-side and it's more straightforward than the client side.

[ahopkins]

@ashleysommer How is this coming? Are we going to be able to get this done this week to sneak it into the release?

[aaugustin]

I'm still tuning a few things in websockets, especially around connection termination and error handling, so it may be safest to aim for the following Sanic release.

[aaugustin]

Also this branch currently depends on the main branch of websockets, so I need to make a release before you can merge it.

[ahopkins]

Good to know. Will plan accordingly

[aaugustin]

@ashleysommer I'm working on an integration guide here: python-websockets/websockets#999

This weekend I reworked the implementation to ensure data_to_send returns the EOF marker (b"") at the right time — it didn't. This shouldn't affect you.

Please note the addition of the close_expected method, which is designed to help with dangling connections. Since you're doing the server side, it's especially helpful in the following circumstances:

• you send a close frame
• due to network unreliability, you never hear about the client again
• you have a dangling TCP connection

In this case, you'd rather time out after 10 seconds than 2 hours (which I believe is the TCP timeout if you don't do anything) :-)

[ahopkins]

@ashleysommer Any update? Are we targeting next release?

[ahopkins]

@ashleysommer update?

[ahopkins]

@aaugustin I spoke with @ashleysommer and he said he was hoping you'd have some of your changes released soon. Is this the case? We are hoping to target a release next month and I'd love to get this in since there are a few websocket issues to address with current implementation.

[aaugustin]

Code & tests for what you need are OK; I'm now working on the docs. I think I can get them done and push a release by the end of August. Would that work for you?

[ahopkins]

by the end of August

That would be amazing 😍

[aaugustin]

Well, I missed my "end of August" deadline for the next websockets release, but hopeful to get it done this week-end.

[ashleysommer]

Finished my big refactoring, splitting out the new websockets code into modules as described by @ahopkins above a few months ago.
All the functionality is in, moving onto testing phase. There are still some excess assert statements hanging around that can be removed once we've thoroughly tested everything.
Similarly, there might be some asyncio.Lock mutexes that can be removed after everything is proved out.
(Note, it's using on websockets github master in lieu of a new websockets release, thats why automated tests will be failing)

[ashleysommer]

I'm not really happy with that websocket requests are handled in the sanic request handler. It still feels like that part of websockets support is kind of hacked in (because it was).

At the moment, a websocket request goes:

TCPServer -> HTTPProtocol -> App Request Handler -> Router -> Request Middleware -> Websocket handler wrapper -> Handshake/Upgrade -> WebsocketProtocol -> Endpoint handler

I understand there isn't much we can do about it at this stage. All requests must be treated as HTTP requests to begin with, and they do still need to go through the router to ensure the request is handled by the correct endpoint handler. And a handshake/upgrade is only done in the handler wrapper, triggered if the handler itself is defined as a websocket handler.

I wonder if we can optimize this pipeline so some extent, by attempting to upgrade the request early (if its a websocket upgrade request), maybe have a websocket-handlers-only section of the router to optimize route lookup, and skipping request-middleware altogether (does that even make sense on a websocket request?) we'd then be able to remove the websocket handler wrapper fn too, because the connection would be already upgraded.

Ideally I'd like something like this:

TCPServer -> HTTPProtocol -> Handshake/Upgrade -> WebsocketProtocol -> WebsocketRouter -> Endpoint handler

But that is a big change

[ashleysommer]

There's still some flake8 issues, looks like the black configuration is not quite compatible with the flake8 rules, in regard to line length.
I'll try to work it out.

[ahopkins]

Great info on the benchmarks.

[ahopkins]

@ashleysommer Nice work on this. It looks like this is a fantastic upgrade to a constant pain point for support.

[aaugustin]

You could also benchmark sanic from a client written in websockets. You can open one or many connections from the client script. Here's a benchmarking script you can take inspiration from: https://github.com/aaugustin/websockets/blob/main/experiments/compression/client.py

It's benchmarking compression, not throughput, but you get the basic structure for connecting one or several clients to a server and sending traffic. The server can either drop the traffic or echo it, as you like.

To stress the server, you can also run multiple copies of the client script. For example, a 6 core machine, I'd try starting 4 clients for 1 server. Then the server will max out 1 CPU core while clients run on 4 CPU cores. This is somewhat valid as long as sanic isn't 4x faster than websockets — else you end up benchmarking the clients instead of the server :-)

[ashleysommer]

Some changes including suggestions from code reviews:

• Use Optional[T] instead of union[T,None]
• Fix mypy type logic errors
• change "is not None" to truthy checks where appropriate
• change "is None" to falsy checks were appropriate
• Add more debug logging when debug mode is on
• Change to using sanic.logger for debug logging rather than error_logger.

[ashleysommer]

Looks like the remaining Flake8 errors are due to line lengths on long strings (mostly in error and debug messages), but Black says those are fine.

[ahopkins]

Looks like the remaining Flake8 errors are due to line lengths on long strings (mostly in error and debug messages), but Black says those are fine.

Black will not know how to break the lines up in the middle of a string. You will need to do that part yourself as elsewhere in the repo:

sanic/sanic/app.py

Line 1011 in 1f06902

raise TypeError(

[ashleysommer]

Final changes:

• Fix flake8 errors on long line lengths of exceptions and debug messages
• Add some new debug messages when websocket IO is paused and unpaused for flow control
• Fix websocket example to use app.static()

[ashleysommer]

Finally.. all green lights

[ashleysommer]

@sanic-org/core-devs Anyone else want to do a review on this?

[aaugustin]

🎉