Feature - Websockets

[eduardovra]

Issue #132

This is an attempt to re-introduce WebSockets support in the project. It's still just a draft, but I wanted to put something together as soon as possible to discuss and shape the solution. The implementation is basically the same as the one present in this branch.
The PR was getting way too big, and I decided to drop the support to broadcast for now. Maybe we can handle this in another PR/issue.

I've left a few TODO comments in the code on points where we should keep an eye, but overall my main questions would be these:

• What's the best approach to instantiate the WebSocketCycle? Right now, I'm picking the protocol based on the lambda event. I think it's the best solution from the user standpoint, as the configuration stays the same as it already is, and we can support both HTTP and WebSocket connections in the same ASGI app. But it's possible to use a separate adaptor class as well.
• The Scope format is slightly different for the WebSocket connection, but I kept using the same as HTTP.
• Binary payloads seem to be unsupported by API GW. They suggest a workaround, though. Not sure if we should try to support this.
• Subprotocols won't be supported, as the application is only called on the MESSAGE event (but the HTTP headers are exchanged on CONNECT).

[jordaneremieff]

What's the best approach to instantiate the WebSocketCycle? Right now, I'm picking the protocol based on the lambda event.

Yeah, that should be fine. Previously I was thinking we might split it out into an additional adapter class that could be configurable in the handler definition, but it's much more convenient and follows the existing convention to detect this from the event context.

The Scope format is slightly different for the WebSocket connection, but I kept using the same as HTTP.

Should be something like this: https://github.com/jordaneremieff/mangum/blob/broadcast/mangum/adapter.py#L195.

Binary payloads seem to be unsupported by API GW. They suggest a workaround, though. Not sure if we should try to support this.

Let's not try support this for now.

Subprotocols won't be supported, as the application is only called on the MESSAGE event (but the HTTP headers are exchanged on CONNECT).

Correct, this will not support subprotocols as the application will be unaware of the actual handshake performed in the connect event.

[eduardovra]

Ok, I think it's already good enough to be reviewed. All tests should pass from now on. The git history is a mess (because I did and undid some things), but we can rebase it later to clean up.
The documentation is still incomplete, I'm planning to finish it when we settle all the details about the code.

I've tested this in AWS with a simple echo server using FastAPI, and a DynamoDB backend. It worked as expected.

Some changes were made, comparing with the implementation present in the broadcast branch:

• I've changed the sigv4 signing implementation to use boto3. At some point, I needed to add signing of DELETE requests and it was easier to do this way, but it introduced boto3 dependency to use WebSockets (along with httpx)
• Changed the backends to work as context managers. It ensures the connection is always going to be closed properly.
• Also on the backends, all of them are going to create the underlying resource if it doesn't exist.

I've tried not to change too much the structure of the code, but unfortunately, because of the differences between WebSocket and HTTP connections, some modifications were necessary.
This PR adds a lot of code and a lot of dependencies to the project, despite my efforts to reduce this. Please take your time to review and ask me any questions you may have.

[jordaneremieff]

Thanks @eduardovra!

I've only had a chance to go through this at a glance, but it's looking really good - well done. I don't have a lot of capacity at the moment, but I'll try to give this a more detailed review when I can.

@koxudaxi if you could give this a review as well it would be very appreciated.

[koxudaxi]

@eduardovra
Thank you for the PR!!
I just checked all changes.
I'm impressed with your knowledge of Python libraries and AWS 🎉
some users may want to use MySQL for managing connections. But, this is not a problem. We can add it easily.
(Also, We should choose Reids or DynamoDB for Performance and the nature of Lambda.)

I could not work tests in my local environment, because Redis-server and PostgreSQL are not installed in my machine.
We should use docker-compose to realize the same environment local and CI.
And you may split the tests into unittest and integration tests for using an external database server.
I put an example of an integration test with PostgreSQL.
https://github.com/koxudaxi/py-data-api/blob/master/scripts/integration_test.sh
https://github.com/koxudaxi/py-data-api/blob/ff081959244cbac9fa901e46effd040889bc0c2e/tests/integration/test_mysql.py#L15

I've tried not to change too much the structure of the code, but unfortunately, because of the differences between WebSocket and HTTP connections, some modifications were necessary.

I guess if many users will use the WebSocket then they will request some new feature.
So, I think the current interface is the best at this time and we only will probably change it for the better by refactoring in the future.

[koxudaxi]

I recommend use docker-compose for these servers.

[eduardovra]

@koxudaxi I moved the backend tests to a subfolder and changed the implementation to use docker-compose as you suggested, but I didn't separate the launch script for the tests as you did on your project. I was afraid it might cause problems in the coverage report. Let me know if you think this is important, and I can take a better look into it.

The way it is right now, the whole suite is taking about 12s to run on my machine, and if I skip the integration tests it takes less than 1 second:

$ pytest --ignore=tests/integration
======================= 95 passed, 1 skipped in 0.86s =======================

[koxudaxi]

@eduardovra
Thank you for updating the unit test. 🙇‍♂️
I think the way is good.

@jordaneremieff
Would you please check the detail of this PR?

[jordaneremieff]

Looks great! I just have some comments for the docs, but otherwise I think this is good to merge.

Probably the main thing is to make sure the minimal example in the docs that works with the new implementation.

[eduardovra]

I've made the changes and added some sections detailing the extra packages that will be required depending on what backend is being used.
The example code was tested using DynamoDB. I haven't tested the other backends in AWS though.

[jordaneremieff]

@eduardovra great, thank you! I'll try the example out either tomorrow or the weekend then merge and push a release.

Well done. :)

[four43]

I'm coming back to this project after a long time and stumbled across this. This is really legit! Great work!