-
Notifications
You must be signed in to change notification settings - Fork 803
Resolve broken filterPingPongs using WebSocketBuild2 with Ember #6036
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
Conversation
…th filter set to false
Not sure how to avoid the binary incompatibility since I needed to change the WebSocketContext api. Open to suggestions or which branch I should PR to for binary incompatible changes? |
Yeah, this is a tricky one :) the good news is that it is possible to make the change binary-compatibly. You'll have to make We recently did this in #5758 and 566fb53 although that was complicated by type erasure problems as well. Binary-breaking changes can go to main, but it won't help you if you are expecting a stable release anytime soon. |
So I looked into this a bit more, and I'm a little confused about why we need to make any changes to
- case Ping(d) =>
+ case p @ Ping(d) =>
//Reply to ping frame immediately
- writeFrame(Pong(d), trampoline) >> handleRead()
+ writeFrame(Pong(d), trampoline) >> F.pure(p) So I think your PR is on the right track ... |
ember-server/shared/src/main/scala/org/http4s/ember/server/internal/WebSocketHelpers.scala
Outdated
Show resolved
Hide resolved
ember-server/shared/src/main/scala/org/http4s/ember/server/internal/WebSocketHelpers.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to approve. Is there an easy test we can add to lock-in this behavior?
I think I should be able to whip one up. I'll get one added this week. 👍🏻 |
@CharlesAHunt Following up. Just don't want to lose track of this one. |
Pings are not passing through when using the Ember server with the WebSocketBuilder2 setting
.withFilterPingPongs(false)
due toWebSocketHelpers.handleIncomingFrames
always mapping them to a None regardless of the filter setting.Fixes #6035