Fixup: Add support for Sec-WebSocket-Protocol to HTTP::WebSocketHandler#1637
Fixup: Add support for Sec-WebSocket-Protocol to HTTP::WebSocketHandler#1637
Conversation
viktorerlingsson
left a comment
viktorerlingsson
left a comment
There was a problem hiding this comment.
handle_sub_protocol and pick_protocol seems to behave differently here. pick_protocol returns the first match, but handle_sub_protocol overwrites on each match and keeps the last one. That could cause a mismatch, no?
|
I don't know if it matters much in this context, but we parse through the headers twice for the same info now (in Besides that, I think it looks good! |
Yeah, I know. I tried to keep it simple and wait for crystal-lang/crystal#16574 , but we might as well do it better and see what that PR ends up with. |
…er (#1637) This fixes some parts that #1621 missed. The specs added in #1621 did only verify that the websocket handler picked the right client handler based on `Sec-WebSocket-Protocol`. What they didn't verify was if `Sec-WebSocket-Protocol` was sent back to the client. Because of how Crystal's `HTTP::WebSocketHandler` and `HTTP::Server:Response#upgrade` works I just copied `HTTP::WebSocketHandler` to make my own fixed version. crystal-lang/crystal#16574 would fix this in the stdlib.
…er (#1637) This fixes some parts that #1621 missed. The specs added in #1621 did only verify that the websocket handler picked the right client handler based on `Sec-WebSocket-Protocol`. What they didn't verify was if `Sec-WebSocket-Protocol` was sent back to the client. Because of how Crystal's `HTTP::WebSocketHandler` and `HTTP::Server:Response#upgrade` works I just copied `HTTP::WebSocketHandler` to make my own fixed version. crystal-lang/crystal#16574 would fix this in the stdlib.
2 participants
WHAT is this pull request doing?
This fixes some parts that #1621 missed.
The specs added in #1621 did only verify that the websocket handler picked the right client handler based on
Sec-WebSocket-Protocol. What they didn't verify was ifSec-WebSocket-Protocolwas sent back to the client.Because of how Crystal's
HTTP::WebSocketHandlerandHTTP::Server:Response#upgradeworks I just copiedHTTP::WebSocketHandlerto make my own fixed version. crystal-lang/crystal#16574 would fix this in the stdlib.HOW can this pull request be tested?
Run specs