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

Switch from bytes to send buffer, to returning on send #91

Closed
pgjones opened this issue Dec 21, 2018 · 3 comments
Closed

Switch from bytes to send buffer, to returning on send #91

pgjones opened this issue Dec 21, 2018 · 3 comments

Comments

@pgjones
Copy link
Member

pgjones commented Dec 21, 2018

Following on from the discussion in #89, I think we should decide whether to keep the internal buffer and the bytes_to_send API or rather to switch to the send method (introduced in #89) returning the bytes to send.
Practically it results in the following API difference from this,

connection.receive_bytes(data)
for event in connection.events():
    ...
send(connection.bytes_to_send())

to

for event in connection.receive_bytes(data):
    if isinstance(event, Ping):
        send(connection.send(Pong())
    elif isinstance(event, Close):
        send(connection.send(Close())
   ...

which would require documentation to explain that a user of wsproto is responsible for ensuring that pings and close are responded to. The latter in my view is advantageous as it is not naively clear that a response should be sent on receipt of Close.

Having thought this through, I think this would be a good change. However it is breaking and would require some care with how it is explained and documented.

@pgjones pgjones mentioned this issue Dec 21, 2018
@Kriechi
Copy link
Member

Kriechi commented Dec 22, 2018

IMHO putting our users in charge of sending Pong's and Close's frames will inevitably lead to broken WebSocket clients and servers on the public internet.

I feel that automatically sending these frames, i.e., protocol maintenance data, greatly benefits everybody because it reduces the required knowledge of the RFC and provides a cleaner / simpler API to use wsproto.

How about we shift the internal buffer to an auto-generation paradigm, i.e., instead of calling send(connection.bytes_to_send()), we could literally auto-generate the protocol maintenance data: send(connection.maintain()), with a very basic:

def maintain(self):
    if close_pending:
        send(connection.send(Close())
        close_pending = False
    elf pong_pending:
       ...

Are there other use cases for maintenance data? Could we incorporate this in the handshake process as well?

@njsmith
Copy link
Member

njsmith commented Dec 23, 2018

IMHO putting our users in charge of sending Pong's and Close's frames will inevitably lead to broken WebSocket clients and servers on the public internet.

I'm not sure if that's true.

There's no way wsproto can force users to follow the RFCs. For example, it's already up to users to correctly coordinate the websocket-level close handshake with the TCP-level close handshake. And for PONGs, we already require them to explicitly check the bytes_to_send buffer after receiving a PING. If they don't do that, then they won't send a prompt PONG, and will be RFC-noncompliant.

If we do switch to requiring users to send PONG and CLOSE frames by hand, then we would certainly want to prominently document that they have to do this and how to do this. But we already need to do that anyway, to explain when and why they have to check bytes_to_send. And we'd also want to make the "how" as easy as possible (e.g., give PONG and CLOSE frames a .response() method that generates the appropriate response frame for you), but that's just regular good design.

@mehaase
Copy link
Member

mehaase commented Jan 7, 2019

As somebody who wrote a downstream WebSocket client based on wsproto, I read the RFC extensively. There are aspects of the protocol that wsproto does not (and should not) concern itself with, such as the TCP teardown that Nathaniel mentioned above. But I also read about the lower level framing stuff that wsproto handles for me because I found it handy to understand what was going on under the hood when, for example, debugging my client by printing out packets to the console or viewing them in Wireshark.

In my opinion, creating a high level abstraction is not the main source of value in wsproto, nor should it be the main goal. The value lies in having most of the protocol cleanly factored, documented, and tested and being able to share that code across multiple downstream projects.

So I suggest that we should not worry too much about RFC compliance and instead focus on ease of use, flexibility, and readability. On these criteria, I like both the response() and maintain() methods, with a slight preference for the former.

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

No branches or pull requests

4 participants