WebSocket#close accepts code and reason arguments

[vladfaust]

Fixes #6469

I saw WebSocket#close specs and they are pretty simple, but I'm absolutely not sure how to convert String.new(bytes[2, bytes[1]]).should eq(message) line to test both reason and code arguments.

Also see https://developer.mozilla.org/ru/docs/Web/API/WebSocket/close.

[vladfaust]

I'm also thinking of parsing on socket close frame as well:

crystal/src/http/web_socket.cr

Line 138 in 60760a5

message = @current_message.to_s

So a developer would write

socket.on_close do |reason, code|
  puts "Closed websocket with #{code} #{reason}"
end

[z64]

Yes, we need to also have close frame decoding, but similar to my last comment, please:

ws.on_close do |code, reason|
  # ..
end

[vladfaust]

Gonna work on it tomorrow

[vladfaust]

Please help me writing socket.on_close { |code, reason| } specs (or write them for me, which is better)

[z64]

Hi @vladfaust, following the http negotiation spec I believe this should be acceptable. This passes for the existing 0.27.2 implementation:

it "calls close handler with code and reason" do
  address_chan = Channel(Socket::IPAddress).new

  spawn do
    http_ref = nil
    ws_handler = HTTP::WebSocketHandler.new do |ws, ctx|
      ws.on_message do
        # ws.close(1000, "goodbye")
        ws.close("goodbye")
        http_ref.not_nil!.close
      end
      ws.send("hello")
    end

    http_server = http_ref = HTTP::Server.new([ws_handler])
    address = http_server.bind_unused_port
    address_chan.send(address)
    http_server.listen
  end

  listen_address = address_chan.receive
  ws = HTTP::WebSocket.new("ws://#{listen_address}")

  ws.on_message do
    ws.send("hello")
  end

  # ws.on_close do |code, reason|
  ws.on_close do |message|
    # code.should eq 1000
    # reason.should eq "goodbye"
    message.should eq "goodbye"
  end

  ws.run
end

Edits for your new spec as commented.

Seeing as there aren't individual specs like this for the other handlers, maybe it should just be added to the spec I linked. I'm not sure though.

I would add whatever you think is best so that we can be sure it works, and then spec style can be addressed in review 👍

[z64]

Just a few other details, the rest I think is okay. I'll leave the rest to the core team, thank you!

[z64]

Just my opinion, but I think def close should send 1000 Normal Closure (delegate close(1000)) per RFC 6455 Sec. 7.4.1

Please document this detail if you accept it

Edit: On second thought, maybe disregard. There is the reserved 1005 close code which is not explicitly sent, but is implicitly received when a client sends a close frame with no body. 🤷‍♂️

[z64]

Actually, shouldn't code be UInt16 everywhere?

a 2-byte unsigned integer (in network byte order) representing a status code with value /code/

[vladfaust]

@Sija see in the discussion above

[Sija]

Can't find it, which discussion?

[z64]

@Sija See:

#7483 (comment)

#7483 (comment)

[Sija]

Shouldn't this be ensure?

[vladfaust]

Makes sense

[Sija]

🏓

[Sija]

ditto

[asterite]

Can't code have a default value other than nil?

[z64]

@asterite In my suggestions I considered close code 1005, which is a special case; it is never meant to be sent by clients, but receiving clients interpret a body-less close frame as close 1005.

I suppose we could do

def close(code : UInt16 = 1005, reason : String? = nil)
  if code == 1005
    send(Bytes.empty, Opcode::CLOSE)
  else
    # send body
  end
end

but then this is confusing (to someone unfamiliar with ws protocol), because reason will never be sent:

ws.close(1005, "reason")
# or
ws.close(reason: "reason")

I think two methods is a cleaner separation of these semantics.

[bew]

Reading https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent#Properties
I think that documented error codes should be available in an enum (with their reason), and a custom close with code+reason should be available for other lib/framework/application errors

[Sija]

@bew IMO using an enum here would be too limiting. Docs especially state that some codes are:

Reserved for use by WebSocket extensions.

or

Reserved for future use by the WebSocket standard.

[z64]

I don't think an enum would be bad.

Enums are not strict in accepting integer values by default, so I don't see how it would be limiting. reason though is just a payload, and is application specific. The name of the code is standard, though, much like HTTP status codes.

Basically, this proposal is #7247 but for websockets (see the enum HTTP::Status). I think this could be done in another PR though.

[vladfaust]

FYI: I'm not very pushy about that because developing Crystal itself is hard on my machine I have this working in my own code. Should I add server specs here? 🤔