Skip to content

Only forward text frames to callbacks#15

Open
maehjam wants to merge 1 commit into
eproxus:mainfrom
stritzinger:bugfix/handle-ping-frames
Open

Only forward text frames to callbacks#15
maehjam wants to merge 1 commit into
eproxus:mainfrom
stritzinger:bugfix/handle-ping-frames

Conversation

@maehjam

@maehjam maehjam commented Apr 25, 2024

Copy link
Copy Markdown
Contributor
  • Ignore other websocket frames like ping, {ping, Data} ...

  • Log when ignoring unexpected frames

Kraft websocket handlers kraft_ws_jsonrpc and kraft_ws_json only handle text frames. This led to function clause errors in our project where we used kraft_ws_jsonrpc and got {ping, binary()} frames in. This PR fixes it by only forwarding the text frames to the kraft websocket handler.

- Ignore other websocket frames like ping, {ping, Data} ...

- Log when ignoring unexpected frames
@maehjam

maehjam commented Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

Note that cowboy will automatically respond for the ping frames, hence the empty return value in that case.

@eproxus

eproxus commented Apr 24, 2025

Copy link
Copy Markdown
Owner

Sorry for being super slow on this one! 🙇

I think a better fix would be to ignore the other frames in kraft_ws_json and kraft_ws_jsonrpc, so that if one wants to actually handle them it can be done in a pure kraft_ws handler. What do you think?

@maehjam

maehjam commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

That is for sure also a way to go. Since kraft_ws_util already implements the websocket ping pong, ignoring those frames in the handlers felt natural to me. Thinking about it again it comes to my mind that cowboy and gun also have build in ping pong handling and still offer developers to handle them in their callbacks, which is a nice feature. So yes, allow to handle them sounds like the better idea. One can even add that possibility to kraft_ws_json and kraft_ws_jsonrpc (with a proper catch to ignore them if it's not implemented in the callback).

@eproxus

eproxus commented May 9, 2025

Copy link
Copy Markdown
Owner

Do you want to make those changes, or can I take your commit and add upon it?

@maehjam

maehjam commented May 22, 2025

Copy link
Copy Markdown
Contributor Author

Sorry for the late response, it's been very busy weeks. Feel free to add upon the changes here.

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