Skip to content

Conversation

bridadan
Copy link
Contributor

I was testing the ws_server and ws_client examples and I hit multiple issues. I've attempted to fix them with these changes, but I'm not 100% sure if this is the "right" way. Thanks for the great library and for considering this change!

  • The websocket client sends two Connection headers:

    HTTP/1.1 GET /
    Host: 127.0.0.1
    Origin: foo.com
    Content-Length: 0
    Connection: Upgrade
    Upgrade: websocket
    Sec-WebSocket-Version: 13
    Sec-WebSocket-Key: h57ZiKx+KAwmKbB+mxR8Ag==
    Connection: Keep-Alive

    The last "Connection: Keep-Alive" is added because the ConnectionType enum does not have an "Upgrade" variant. Adding the variant prevents this issue.

  • If the websocket client does not receive a "Content-Length: 0" header in the server's response, an error is returned stating "Unknown body type in a response with a Keep-Alive connection. This is not allowed." This occurs with the example client using the "websockets.chilkat.io/" service. This is corrected by assuming a "BodyType::ContentLen(0)" for the response when no other BodyType is provided and the "Connection: Upgrade" header is present.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 20, 2025

Actually, your fix looks exactly what we should do!
(And yes - sorry for the headache - I must admit I forgot to test the ws_client/server code with the edge-http changes that introduced the notion of ConnectionType!)

If you fix the formatting issues and the CI passes, I'll merge asap.

- The websocket client sends two Connection headers:

    HTTP/1.1 GET /
    Host: 127.0.0.1
    Origin: foo.com
    Content-Length: 0
    Connection: Upgrade
    Upgrade: websocket
    Sec-WebSocket-Version: 13
    Sec-WebSocket-Key: h57ZiKx+KAwmKbB+mxR8Ag==
    Connection: Keep-Alive

  The last "Connection: Keep-Alive" is added because the ConnectionType
  enum does not have an "Upgrade" variant. Adding the variant prevents
  this issue.

- If the websocket client does not receive a "Content-Length: 0" header
  in the server's response, an error is returned stating "Unknown body
  type in a response with a Keep-Alive connection. This is not allowed."
  This occurs with the example client using the
  "websockets.chilkat.io/" service. This is corrected by assuming a
  "BodyType::ContentLen(0)" for the response when no other BodyType is
  provided and the "Connection: Upgrade" header is present.
@ivmarkov ivmarkov merged commit 2cd767f into sysgrok:master Jan 20, 2025
2 checks passed
@bridadan
Copy link
Contributor Author

Thanks!

@ivmarkov
Copy link
Collaborator

Ill release a new edge-http 5.x patch as well these days. In theory, it should be a breaking release, because we added a new member to a #[non_exhaustive] enum but maybe we can squint a little for that...

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

Successfully merging this pull request may close these issues.

2 participants