Skip to content
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

adds NIOWebSocketFrameAggregator to buffer fragmented WS frames #50

Merged
merged 2 commits into from
Jul 15, 2021

Conversation

dnadoba
Copy link
Member

@dnadoba dnadoba commented May 5, 2021

Motivation:

We want support frame fragmentation at the WebSocket level

Modifications:

add NIOWebSocketFrameAggregator to channel pipeline in front of our own channel handlers

Result:

Fragmented WebSocket Frames are now supported.

Discussion

What are good default values for minNonFinalFragmentSize and maxAccumulatedFrameCount for WebSocket?
The equivalent of minNonFinalFragmentSize in RSocket would be 64 bytes but I don't know if this is appropriate for WebSocket as well.
I have set minNonFinalFragmentSize by default to 0 and maxAccumulatedFrameCount to Int.max for now.

minNonFinalFragmentSize and maxAccumulatedFrameCount are part of WSTransport and not ClientConfiguration because they are specific to WebSocket. I kind of want them to be part of ClientConfiguration. I initially thought that we could just add these properties to ClientConfiguration.Fragmentation and even use the same values for WebSocket fragmentation and RSocket fragmentation. But that would mean one can not configure fragmentation separately for WebSocket and for RSocket... Maybe that's good thing, less configuration one has to worry about is in general a good thing. However, I can't say for sure that RSocket fragmentation and WebSocket fragmentation should always have the same buffer limits.

Signed-off-by: David Nadoba <dnadoba@gmail.com>
Signed-off-by: David Nadoba <dnadoba@gmail.com>
@dnadoba dnadoba marked this pull request as ready for review May 24, 2021 02:39
Copy link
Member

@OlegDokuka OlegDokuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@OlegDokuka OlegDokuka changed the title use NIOWebSocketFrameAggregator to buffer fragmented WebSocket frames adds NIOWebSocketFrameAggregator to buffer fragmented WS frames Jul 15, 2021
@OlegDokuka OlegDokuka merged commit 185c2c8 into main Jul 15, 2021
@OlegDokuka OlegDokuka deleted the websocket-fragmentation branch July 15, 2021 09:15
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.

3 participants