Add handling for fragmented frames in WebSockets

[mox692]

resolve #7073.

This PR adds handling for fragmented frames in WebSockets.

Currently, if a user wants to handle fragmented frames, they must explicitly include defragmentation logic in their receive handler. With this PR, the user can ensure that no fragmented frames arrive within the receive pipe they provide, and they no longer need to consider fragmentation.

This feature is enabled by default and can be turned off with withDefragFrame method.

RFC 6455; 5.4. fragmentation

[armanbilge]

This is looking great so far, very nice work!

[mox692]

Now I’m considering the configurable part, and I have a image like this:

By default, we aggregate the incoming fragmented frame by using aggregateFragment method. Also, user can provide their own custom WebSocketFrameAggregator with their own aggregateFragment method. In the latter case, user can write like this,

def httpApp[F[_]](wsBuilder: WebSocketBuilder2[F]): HttpApp[F] =
  HttpRoutes
    .of[F] {
      case GET -> Root / “xxx” =>
        ...

        wsBuilder
          // User can provide their own WebSocketFrameAggregator implementation.
          .withWebSocketFrameAggregator(CustomWebSocketFrameAggregator.apply[F]())
          .build(sendReceive)

So users who don’t want to implicitly aggregate fragments can provide a WebSocketFrameAggregator that does nothing.

@armanbilge
What do you think about this? Do you have any other good ideas?
If we want to achieve this, we would need to make changes to WebSocketBuilder2, but I’m not sure how costly that would be.

Of course, feedback on other parts is also welcome!

[armanbilge]

I like the idea of adding this as a configuration on WebSocketBuilder2, if we can do it without breaking compatibility!

I am wondering if it is sufficient to just expose wsb.withDefrag/wsb.withoutDefrag configuration on the builder. I don't think we need to let the user customize the aggregator in the builder because they can just transform the stream in their receive implementation, is that right?

[mox692]

Oh, yes, I agree.
It is probably not necessary to make the user provide something that only performs aggregation, and that approach seems much simpler to implement.

Thank you, I will continue working on wsb.withDefrag / wsb.withoutDefrag feature.

[mox692]

Hmm, it seems that adding a parameter to a sealed abstract class could break binary compatibility, even if the constructor of that class is marked as private. 🤔
I need to think of a slightly different approach…

[mox692]

It looks ok to me, so I've just changed the PR status :)

[armanbilge]

Everything is looking very nice, for happy-path 😄 I have some minor comments about naming and optimizations.

However, it's less clear how the not-so-happy path is handled e.g. if continuation frames are missing or of the wrong type. Actually I realized that maybe we don't need to consider this here: the server should be rejecting those anyway before they reach the defragmenter. If that's the case, then let's document on here that this defragmenter expects a valid sequence and will produce wrong results otherwise.

[mox692]

@armanbilge
Thanks for your review.
There are still some works to be done, but my response may be a little delayed.

[mox692]

I made changes in two commits, 9e3eca5, 9c39fa6, to clarify the processing for unhappy paths and added tests for them.

Regarding unhappy path, I am considering unifying the behavior to "not perform defragmentation and pass input to output as is", but I would like to hear your opinions.

[armanbilge]

I am considering unifying the behavior to "not perform defragmentation and pass input to output as is", but I would like to hear your opinions.

Sorry, I misunderstood: is this "unified behavior" the current implementation in your PR (it seems like it?) or is this another possible implementation that you are proposing?

In any case, I like what you've done here! Thanks for all your work on this :)

[mox692]

@armanbilge
Sorry for the confusion!
The former, I intend to incorporate this into our implementation already.

Before receiving feedback from Arman, there were cases where there was no clear behavior for the operation when an invalid sequence was passed, e.g. some sequence pattern being defragmented and others not. Therefore, in the above commit, I tried to unified the behavior to not defragment if an invalid sequence related to fragmentation comes.

Or am I missing something?

[armanbilge]

Sorry for the slow review cycle on this. There is one more topic that I've been considering, from the linked RFC. I wonder if there is a good way to check for these conditions (reserved bit values or extensions) and automatically disable defragmentation in that case.

   o  An intermediary MUST NOT change the fragmentation of a message if
      any reserved bit values are used and the meaning of these values
      is not known to the intermediary.

   o  An intermediary MUST NOT change the fragmentation of any message
      in the context of a connection where extensions have been
      negotiated and the intermediary is not aware of the semantics of
      the negotiated extensions.  Similarly, an intermediary that didn't
      see the WebSocket handshake (and wasn't notified about its
      content) that resulted in a WebSocket connection MUST NOT change
      the fragmentation of any message of such connection.

[mox692]

Good point, thank you.
I think at least the following support is necessary to achieve the above.

1. Support Sec-WebSocket-Extensions (currently this header is not explicitly supported)
2. Make reserved-bits visible from WebSocketFrame (reserved-bits can be set in any WebsocketFrame)

Should these be addressed in a separate issue first?

[armanbilge]

Thanks for researching that! I agree, looks like it will need some follow-up work. Anyway, hopefully those are more rare usecases 🤔

In that case, everything here looks good to me! Nice work!