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#close accepts code and reason arguments #7483

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions spec/std/http/web_socket_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,16 @@ describe HTTP::WebSocket do
end

describe "close" do
it "closes with message" do
message = "bye"
it "closes with code and response" do
response = "bye"
code = 4000_i16
io = IO::Memory.new
ws = HTTP::WebSocket::Protocol.new(io)
ws.close(message)
ws.close(code, response)
bytes = io.to_slice
(bytes[0] & 0x0f).should eq(0x8) # CLOSE frame
String.new(bytes[2, bytes[1]]).should eq(message)
IO::ByteFormat::NetworkEndian.decode(Int16, bytes[2, 3]).should eq(code)
String.new(bytes[4, bytes[1] - 2]).should eq(response)
end

it "closes without message" do
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/tools/playground/server.cr
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ module Crystal::Playground
origin = context.request.headers["Origin"]
if !accept_request?(origin)
@logger.warn "Invalid Request Origin: #{origin}"
ws.close "Invalid Request Origin"
ws.close 1002, "Invalid Request Origin"
else
@sessions_key += 1
@sessions[@sessions_key] = session = Session.new(ws, @sessions_key, @port, @logger)
Expand Down
29 changes: 22 additions & 7 deletions src/http/web_socket.cr
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class HTTP::WebSocket
def on_binary(&@on_binary : Bytes ->)
end

def on_close(&@on_close : String ->)
def on_close(&@on_close : Int16?, String? ->)
end

protected def check_open
Expand Down Expand Up @@ -90,18 +90,26 @@ class HTTP::WebSocket
end
end

def close(message = nil)
# Closes the websocket.
def close
return if closed?
@closed = true
@ws.close(message)
@ws.close
end

# Closes the websocket with *code* and *reason*.
def close(code : Int16, reason : String? = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, shouldn't code be UInt16 everywhere?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sija see in the discussion above

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't find it, which discussion?

Copy link
Contributor

Choose a reason for hiding this comment

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

return if closed?
@closed = true
@ws.close(code, reason)
end

def run
loop do
begin
info = @ws.receive(@buffer)
rescue IO::EOFError
@on_close.try &.call("")
@on_close.try &.call(nil, nil)
vladfaust marked this conversation as resolved.
Show resolved Hide resolved
break
end

Expand Down Expand Up @@ -135,9 +143,16 @@ class HTTP::WebSocket
when Protocol::Opcode::CLOSE
@current_message.write @buffer[0, info.size]
if info.final
message = @current_message.to_s
@on_close.try &.call(message)
close(message) unless closed?
begin
code = @current_message.read_bytes(Int16, IO::ByteFormat::NetworkEndian)
reason = @current_message.gets_to_end
@on_close.try &.call(code, reason)
rescue IO::EOFError
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be ensure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

🏓

@on_close.try &.call(nil, nil)
vladfaust marked this conversation as resolved.
Show resolved Hide resolved
ensure
close unless closed?
end

@current_message.clear
break
end
Expand Down
15 changes: 9 additions & 6 deletions src/http/web_socket/protocol.cr
Original file line number Diff line number Diff line change
Expand Up @@ -229,12 +229,15 @@ class HTTP::WebSocket::Protocol
end
end

def close(message = nil)
if message
send(message.to_slice, Opcode::CLOSE)
else
send(Bytes.empty, Opcode::CLOSE)
end
def close
send(Bytes.empty, Opcode::CLOSE)
Copy link
Contributor

@z64 z64 Feb 27, 2019

Choose a reason for hiding this comment

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

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. 🤷‍♂️

end

def close(code : Int16, reason : String? = nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

slice = Bytes.new(sizeof(Int16) + (reason ? reason.bytesize : 0))
IO::ByteFormat::NetworkEndian.encode(code, slice)
reason.to_slice.copy_to(slice + sizeof(Int16)) if reason
send(slice, Opcode::CLOSE)
end

def self.new(host : String, path : String, port = nil, tls = false, headers = HTTP::Headers.new)
Expand Down