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

Websocket sansio implementataion #2060

Open
wants to merge 64 commits into
base: master
Choose a base branch
from

Conversation

gourav-kandoria
Copy link

@gourav-kandoria gourav-kandoria commented Jul 27, 2023

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

task details - #1908

Checklist for @Kludex

  • data_receive shouldn't be using Exception. What is the right exception there?
  • Check if max_size is being tested, and if not, test it. EDIT: I've checked it - it's not being tested.
  • Add WebSocketsSansIOProtocol test for max_size.
  • Should we remove CONT and PONG related code?
  • Add documentation.
  • Increase the coverage number.
  • Add --ws websockets-sansio.
  • Check if Websocket sansio implementataion #2060 (comment) is true.
  • Answer the following questions:
    • Should we deprecate websockets legacy version?
    • Should we add a UserWarning saying that it's experimental, and asking users to comment in a discussion X?

@Kludex
Copy link
Member

Kludex commented Jul 27, 2023

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

@gourav-kandoria
Copy link
Author

sure

@gourav-kandoria
Copy link
Author

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

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll review on the weekend.

uvicorn/protocols/websockets/websockets_sansio_impl.py Outdated Show resolved Hide resolved
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@valentin994
Copy link

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 😄

if self.handshake_initiated and not self.close_sent:
self.queue.put_nowait({"type": "websocket.disconnect", "code": 1006})

def data_received(self, data: bytes) -> None:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://websockets.readthedocs.io/en/stable/howto/sansio.html
if I'm following this right after receiving data you should call receive_eof()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valentin994 @Kludex we don't have to call receive_eof() after receiving data. Because, protocol ensures that (https://docs.python.org/3/library/asyncio-protocol.html#asyncio.Protocol.data_received) data_received will only be called only when the data is there. In case when eof is received from client side, the transport will close and connection_lost will be called.

@gourav-kandoria
Copy link
Author

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

@Kludex
Copy link
Member

Kludex commented Aug 8, 2023

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.

self.conn.receive_data(data)
except Exception:
self.logger.exception("Exception in ASGI server")
self.transport.close()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lines(117-119) are not being covered. Because, I guess in the test suite there not any test which after connection establishment sends data which causes receive_data to throw exception such as data which don't follow websocket spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to test it. It's unlikely that we want Exception there as well.

@Kludex Kludex force-pushed the websocket-sansio-implementataion branch from 12bb2d2 to 64d6eb7 Compare December 26, 2023 13:00
@gourav-kandoria gourav-kandoria deleted the websocket-sansio-implementataion branch January 8, 2024 17:19
@Kludex
Copy link
Member

Kludex commented Jan 8, 2024

Why?

@gourav-kandoria gourav-kandoria restored the websocket-sansio-implementataion branch January 13, 2024 07:25
@gourav-kandoria
Copy link
Author

gourav-kandoria commented Jan 13, 2024

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 Kludex reopened this Aug 8, 2024
@Kludex
Copy link
Member

Kludex commented Aug 8, 2024

cc @aaugustin

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

Successfully merging this pull request may close these issues.

4 participants