Skip to content

Extract WebSocket#do_ping, #do_close helper methods for overrides#15545

Merged
straight-shoota merged 3 commits intocrystal-lang:masterfrom
luislavena:websocket-default-onping-callback
Apr 18, 2025
Merged

Extract WebSocket#do_ping, #do_close helper methods for overrides#15545
straight-shoota merged 3 commits intocrystal-lang:masterfrom
luislavena:websocket-default-onping-callback

Conversation

@luislavena
Copy link
Contributor

Before this change, when using #on_ping callback and defining your own block of code to handle it, resulted in two pong frames being sent to the other site of the WebSocket.

This change setups a default ping <-> pong response cycle but once you override it, it will use yours instead.

Thank you
❤️ ❤️ ❤️

Before this change, when using `#on_ping` callback and defining your
own block of code to handle it, resulted in two `pong` frames being sent
to the other site of the WebSocket.

This change setups a default ping <-> pong response cycle but once you
override it, it will use yours instead.
@luislavena luislavena changed the title Avoids duplicate responses to ping frames Avoids duplicate responses to ping frames (WebSocket) Mar 8, 2025
@luislavena
Copy link
Contributor Author

This has been reported in #8435, but this PR only deals with ping events.

@ysbaddaden
Copy link
Collaborator

⚠️ This is a breaking change: the current behavior is that the PONG frame will be automatically sent after the block is run unless the websocket is closed, as per RFC 6455.

@ysbaddaden
Copy link
Collaborator

These are WebSocket protocol details that applications shouldn't bother with: the handlers allow you to do something on the application side.

I don't think there is anything to fix but the documentation.

@ysbaddaden
Copy link
Collaborator

Oh, I missed the WebSocket proxy that forwards the ping and pong to the other side, expecting each side to talk to each other.

I guess it's an acceptable claim. Edgy, though.

@luislavena
Copy link
Contributor Author

Yeah @ysbaddaden, this proxy thing is a bit tricky for a standard library (aka: general) usage.

I might override run in my own class and deal with that privately instead.

🤔

@straight-shoota
Copy link
Member

Perhaps we could extract the implicit ping and pong behaviour into helper methods?
That would allow implementations to inherit from WebSocket in order to make protocol-level adjustments. They could override specific behaviour without having to override the entire #run method.

Per RFC 6455, for any PING frame, a similar PONG frame with same payload
is expected as response.

Previous change broke that if the developer used `#on_ping` callback to
setup their own event, resulting a broken implementation.

This extracts both ping and close events as private helpers to ease
reutilization/override.
@luislavena
Copy link
Contributor Author

Hello folks, thank you for your patience.

I decided go with @straight-shoota recommendation and extracted ping and close elements as do_ping and do_close private methods.

Thank you again for your time.
❤️ ❤️ ❤️

This is no longer relevant and avoids breaking RFC 6455
@straight-shoota straight-shoota added this to the 1.17.0 milestone Apr 17, 2025
@straight-shoota straight-shoota changed the title Avoids duplicate responses to ping frames (WebSocket) Extract WebSocket#do_ping, #do_close helper methods for overrides Apr 18, 2025
@straight-shoota straight-shoota merged commit 3233687 into crystal-lang:master Apr 18, 2025
33 of 34 checks passed
@luislavena luislavena deleted the websocket-default-onping-callback branch May 19, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants