Websocket improvements

[devnote-dev]

Crystal's HTTP::WebSocket is still lacking in some key parts, which I will go over individually:

• No overview section or basic usage examples
• Incorrect/misleading method naming
• Lack of explicit control

I believe that the first point is fairly self-explanatory: there is very little information about the usage of websockets in the documentation – not even an overview section! This is obviously not helpful for people that are new to websockets or their usage in Crystal. In particular, Crystal's websocket implementation does not have an on_open method that is common in other websocket libraries in other languages. This is due to the fact that the websocket connection is opened in instantiation (the new method), so you can simply send to websocket afterwards; nowhere is this distinction made. This could be rectified by adding a simple overview section with examples for websockets, even if it is minimal.

My second point is targeted at the run method which states:

Continuously receives messages and calls previously set callbacks until the websocket is closed.

While it does say it receives messages and calls the packet callbacks (binary, message, etc), it's quite obvious how one—like a newcomer to Crystal—can misinterpret this. Had this not been stated, the name alone implies that it runs the websocket, which is not the case. The name should reflect its actual purpose which is to listen for incoming packets and handle them accordingly. The run method could be deprecated and use the new method under the hood until this change is phased out. The docs should also warn that the method is blocking/fiber-demanding, which is not immediately obvious until you try to execute code after ws.run (related: #11413).

My third point is mostly extracted from #8435 with additional implementation details, but to summarise: there is no way to manually manage a websocket connection as the run method automatically handles incoming packets and responses for pings/pongs. There should be an explicit receive method to receive an incoming packet, and an on_packet callback handler which users can use to handle incoming packets regardless of whether the user is managing the websocket manually. Ideally, the run (or listen I should say) method would use receive under the hood to prevent code duplication, and in general the code should not have to change too much to accommodate for these features (they're also non-breaking which is always a plus).

WDYT?

[z64]

Crystal's websocket implementation does not have an on_open method that is common in other websocket libraries in other languages.

The reason other languages have on_open (or IO callbacks in general) is usually because they do not have async/await, or otherwise have async IO integrated into the language itself, and necessitate the use of a callback to hook into the IO event system. (WebSocket API greatly predates standardized async/await in JS)

As you have hinted at, there is no need for for on_XYZ callbacks + run at all in Crystal. It is incidentally how the original API was designed, or maybe some very early versions of Crystal influenced it. And there has already been motions to add another interface that does not require callbacks at all, that would obviate some of these issues, like you have described. But they have all stalled.

See other related issues/PRs:

• WebSockets should have blocking methods #5600
• WebSocket: add blockable receive and receive_raw methods #8024
• New http::WebSocket#read to avoid running an infinite loop with #run #8392

[devnote-dev]

As you have hinted at, there is no need for for on_XYZ callbacks + run at all in Crystal. It is incidentally how the original API was designed, or maybe some very early versions of Crystal influenced it.

I think there is a slight miscommunication here on my part: I'm not saying that the on_xyz methods should be removed, but it shouldn't be the only way to do things.

I like Watzon's example provided in #5600 but there are two important problem which Asterite points out: you must handle ping/pong manually and you must close when receiving a close code. I don't think that users should be forced to handle this explicitly unless that is their intention. There is also another issue pointed out by Asterite with the following code example:

ws = HTTP::WebSocket.new("ws://echo.websocket.org", "/")

ws.send("hello", "This is a message")
message = ws.receive

But that's wrong! There's no guarantee that ws.receive will get the proper message. It might be a ping. It might be a close. It can be anything, really.

Keeping the callback methods would resolve this, specifically an on_packet one for the above example.

[z64]

I'm not saying that the on_xyz methods should be removed

Neither am I. I am just giving context that the callback design was not necessary in Crystal, wherein other languages it was necessary, thus they have on_open but Crystal does not. (In other words, it is not a 1:1 comparison to other WS APIs)

Even if they were to be removed, the callbacks can be trivially implemented on top of the blocking interface.

[straight-shoota]

I think it's clear that Websocket needs an update. But so far nobody has gotten around to propose a complete API overhaul design, let alone implement it. I guess there are just bigger fish to fry and the API we have does the job (although it's not pretty).
But maybe this new initiative can lead to some actual improvements happening =) Thanks for starting this 🙇

you must handle ping/pong manually and you must close when receiving a close code. I don't think that users should be forced to handle this explicitly unless that is their intention

I agree that users should not have to deal with this, but I don't understand why this would need to leak outside of #receive?
It should be possible to handle ping (send pong and wait for the next message) and close (raise exception or return nil) completely internally.

Even if they were to be removed, the callbacks can be trivially implemented on top of the blocking interface.

Yes, I agree that would probably be a good solution.

[devnote-dev]

I agree that users should not have to deal with this, but I don't understand why this would need to leak outside of #receive?
It should be possible to handle ping (send pong and wait for the next message) and close (raise exception or return nil) completely internally.

My apologies, the message = ws.receive part threw me off a bit. You're right, #receive could be used to handle incoming packets explicitly as well as internally (as it currently does).

But so far nobody has gotten around to propose a complete API overhaul design, let alone implement it.

I would be happy to do so, for the API design I like some of the points in Watzon's issue and #8024, however:

• I think that wrapping the packet messages in structs is unnecessary. If there were more data in these packets then this would make sense, but it literally comes down to being either a String, Bytes or Nil so we should keep it simple
• #receive_raw (if implemented) should return PacketInfo, if you want to do anything lower-level than that then you should just use HTTP::WebSocket::Protocol directly
• #receive should parse packets from #receive_raw into the string/bytes/etc object and return a tuple of the packet type and the content. We could make use of HTTP::WebSocket::Protocol::OpCode here rather than multiple single-value structs (see example below)
• #run should be deprecated in favor of #listen which uses #receive to handle incoming messages and callback handlers
• I'm not entire sure what the purpose of #stream is, even when looking at the code it just seems to be yielding an IO for writing to the socket and nothing else (correct me if I'm wrong). If there's no actual use-case for this then it should be deprecated
• HTTP::WebSocket::Protocol::OpCode should be documented as it would no longer exclusively for the internal API. We should be able to move it to HTTP::WebSocket::OpCode for consistency

# http/web_socket.cr
def receive : {Protocol::OpCode, String | Bytes | CloseCode | Nil}
  info = recieve_raw
  @current_message.write @buffer[0, info.size]

  case info.opcode
  when .binary?
    {:binary, @current_message.to_slice}
  when .close?
    return {:close, nil} unless info.final?

    @current_message.rewind
    code = CloseCode::NoStatusReceived

    if @current_message.size >= 2
      code = @current_message.read_bytes(UInt16, IO::ByteFormat::NetworkEndian).to_i
      code = CloseCode.new(code)
    end

    {:close, code}
  else
    {info.opcode, @current_message.to_s}
  end
end

# program.cr
ws = HTTP::WebSocket.new("ws://echo.websocket.org", "/")
ws.send("hello")

opcode, message = ws.receive
case opcode
when .ping?
  # ...
when .pong?
  # ...
end

[straight-shoota]

Is exposing OpCode even necessary?
I think the return type of #receive could sufficiently denote the type of message.

[devnote-dev]

I'd say yes purely for documentation reasons – it doesn't even need to have doc comments, just removing :nodoc: so that it is present is all.

Edit: OpCode is under nodoc because the Protocol class is nodoc, so moving it to HTTP::WebSocket::OpCode would fix this.

[straight-shoota]

Sry, that was too unspecific. I mean as a part of the return value of #receive.

[devnote-dev]

Without it there would be no way to determine which packets are pings, pongs or messages. #receive isn't exclusively for text or binary messages, it's parsing everything from #receive_raw.

[straight-shoota]

Maybe this explains better what I mean:

# http/web_socket.cr
record Ping, message : String
record Pong, message : String

def receive : String | Bytes | {CloseCode, String} | Ping | Pong
  # ...
end

# program.cr
ws = HTTP::WebSocket.new("ws://echo.websocket.org", "/")
ws.send("hello")

case message = ws.receive
in Bytes # equivalent to on_binary
  # ...
in String # equivalent to on_message
  # ...
in Tuple(CloseCode, String) # equivalent to on_close
  # ...
in Ping # equivalent to on_ping
in Pong # equivalent to on_pong
end

[devnote-dev]

I didn't know this was possible with case..in, I really like it though! Maybe we should go with this instead.

[devnote-dev]

@straight-shoota Is there anything else that needs to be added or changed to this? If not then I can start working on the implementation.

[straight-shoota]

Yeah, I guess it's fine to start on an implementation. It should be simple enough that it's easier to just go write something. And it won't be a big deal if we end up iterating some changes on the initial proposal.