Skip to content

Conversation

CharlesAHunt
Copy link
Contributor

@CharlesAHunt CharlesAHunt commented Feb 10, 2022

Pings are not passing through when using the Ember server with the WebSocketBuilder2 setting .withFilterPingPongs(false) due to WebSocketHelpers.handleIncomingFrames always mapping them to a None regardless of the filter setting.

Fixes #6035

@CharlesAHunt
Copy link
Contributor Author

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?

@armanbilge
Copy link
Member

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 WebSocketContext into an ordinary class and manually restore all the missing case class methods (if you are good at reading MiMa issues, they will guide you with which methods to add). Also because case classes are encoded differently on Scala 2/3 it will necessitate splitting sources.

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.

@armanbilge
Copy link
Member

armanbilge commented Feb 12, 2022

So I looked into this a bit more, and I'm a little confused about why we need to make any changes to WebSocketContext at all actually.

  1. The PR that added the filterPingPongs API also implemented support for this feature in Blaze. So it's possible to implement this without modification to WebSocketContext.
    Added new WS API which allows receiving Ping messages from the client #3136
  2. In fact, Blaze's implementation doesn't appear to use filterPingPongs at all. AFAICT the parameter is used only in WebSocketBuilder2:
    val finalReceive: Pipe[F, WebSocketFrame, Unit] =
    if (filterPingPongs)
    _.filterNot(isPingPong).through(receive)
    else
    receive
  3. The PR adding this feature made the following change to Blaze:
- 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 ...

Copy link
Member

@armanbilge armanbilge left a 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?

@CharlesAHunt
Copy link
Contributor Author

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. 👍🏻

@ChristopherDavenport
Copy link
Member

@CharlesAHunt Following up. Just don't want to lose track of this one.

@armanbilge armanbilge merged commit b30c07e into http4s:series/0.23 May 2, 2022
@armanbilge armanbilge mentioned this pull request May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ember Server is returning a None for Socket Pings breaking .withFilterPingPongs(false)
3 participants