encode / uvicorn

An ASGI web server, for Python. 🦄
https://www.uvicorn.org/
BSD 3-Clause "New" or "Revised" License
8.57k stars 745 forks source link

Websocket sansio implementataion #2060

Open gourav-kandoria opened 1 year ago

gourav-kandoria commented 1 year ago

This PR provides the implementation of websocket protocol using websockets sansI/O layer.

task details - https://github.com/encode/uvicorn/issues/1908

Checklist for @Kludex

Kludex commented 1 year ago

If you make the pipeline green, I can review it.

gourav-kandoria commented 1 year ago

sure

gourav-kandoria commented 1 year ago

@Kludex Could you please review it. all checks are passing now

valentin994 commented 1 year ago

I'm going to explain +- how is going to be my review process, so you can try to speed it up:

1. I'll run the test suite, and check the coverage, and then I'll try to understand why some lines are not covered.

2. I'll check the type hints, and make sure that they make sense.

3. There are some tests that only work for `wsproto` or for `websockets`. I'm going to make sure that we are also running this protocol implementation where it makes sense.

I was looking at this task as well, but as gourav-kandoria already did it I can help with pointing out few semantics or if I see something else. If you don't mind 😄

gourav-kandoria commented 1 year ago

okay sure, got it. till then I will also go through the code at my end.

Kludex commented 1 year ago

FYI, if you want to speed up the review process here, I need the lines on the report for the added file to be covered, or to understand why they aren't.

Screenshot 2023-08-08 at 07 42 48

I'll eventually come to this PR even if you don't do it, but it may take some time.

gourav-kandoria commented 1 year ago

@Kludex as per your suggestion. found out and have mentioned the reason for those missing lines in the comments open for discussion about any issue you might see

Kludex commented 1 year ago

I'll modify the description to set what is missing for this to be merged.

Kludex commented 1 year ago

@aaugustin Do you also want to review it?

I've already reviewed. I need to check the points on the checklist, and help there is also wanted - some are just questions. 🙏

aaugustin commented 1 year ago

I'll try to find time this week. If you haven't heard from me by the end of the week, please consider that I timed out :-)

Kludex commented 1 year ago

I think we are almost there. 👍

gourav-kandoria commented 1 year ago

max_size is not being tested on WebSocketSansIOProtocol - There's a test that uses max_size on the test_websockets.py.

The Exception part, we need to add the right exception, and test it.

max_size is not being tested on WebSocketSansIOProtocol - There's a test that uses max_size on the test_websockets.py.

The Exception part, we need to add the right exception, and test it.

@Kludex raised an issue regarding this https://github.com/python-websockets/websockets/issues/1397#issue-1877077219

gourav-kandoria commented 1 year ago

Hi @Kludex can we proceed on it.

Kludex commented 10 months ago

Why?

gourav-kandoria commented 10 months ago

oh my bad. branch got deleted by mistake. Due to bandwidth issue, I won't be able to contribute on this. That's why closed the pr. Although If you want to continue on this. You may refer this branch https://github.com/gourav-kandoria/uvicorn/tree/websocket-sansio-implementation-2. adding additional typing checks would be sufficient as the code in this branch is following complete websocket asgi specs and passing all test cases

Kludex commented 3 months ago

cc @aaugustin