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

Custom parser support #231

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

FabianHummel
Copy link

Motivation

I am developing a high performance realtime collaboration server and noticed a problem when sending / receiving multiple packets from and to the server with the default packet parsing implementation. Because of the way socketioxide handles messages asynchronously, websocket frames may get messed up resulting in the wrong order, sometimes crashing the client or the server (although that needs more testing).

Solution

The obvious option would be to fix the underlying issue of ensuring all packets are received in the correct order, but I had to find a quicker and more performant fix to be able to use socketioxide as my primary server implementation. With this PR, I chose to implement custom packet parsing and enabling developers to implement their own protocols or improve packet size / performance by using msgpack.

Open Todos

  • Have a parser instance per each client / socket so that collecting binary packets is easier and is not required to do via SocketData::partial_bin_packet, and rather directly inside the parser.
  • Cleaner API, currently the decode_msg function returns a Result and the decode_bin returns an Option which is inconvenient.
  • More freedom with encoding and decoding packets, a lot of logic is still hardcoded into the client struct.
  • Unit tests. I commented them out because I had issues with getting them work.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@Totodore
Copy link
Owner

This is a good idea, can you create a corresponding tracking issue for dynamic parsers please :).

@Totodore
Copy link
Owner

Also can you explain more in detail your issue with message ordering and web socket frames? Maybe provide an example? I don't completely understand your issue and in what way a msgpack parser may solve it.

@Totodore Totodore added enhancement New feature or request socketio-v4 It is only about the version 4 of the socket.io protocol socketio-v5 It is only about the version 5 of the socket.io protocol labels Jan 16, 2024
@tausifcreates
Copy link
Contributor

tausifcreates commented Jan 16, 2024

This is my question, as well. If message ordering is not sequential (because async), is it not upto the client to re-order them based on different criteria (like timestamp)?

@FabianHummel
Copy link
Author

FabianHummel commented Jan 16, 2024

I'll try to explain the issue I have been experiencing with socketioxide + socket.io-client. When sending two independent packets in a very tiny timeframe, they both arrive pretty much the same time at the server. This seems to be working, all packets arrive at the server and are handled just fine, but my guess is that the ack or any other packets sent back to the client may get messed up in order because they are sent by two parallel-running tokio threads. I'll try to visualize. This is socketioxide:

image

And this is Socket.io: Note that the packets are sent back exactly the same as they were received. (At least I have not had any issues with my previous Socket.io server in this regard)
image

Still, I am just guessing that this causes the problem. I did not yet do extensive research on this problem. The solution with msgpack is also described on the official Socket.io documentation here. This way, it does not really matter in what order the packets are sent and received, as one packet is equal one ethernet frame and neither side needs to wait until all packets have arrived, ultimately solving the underlying issue (although not the preferred way in most cases).

Pros: packets with binary content are sent as one single WebSocket frame (if the WebSocket connection is established)

@Totodore
Copy link
Owner

Okay so now I understand better. It is due to the fact that socket.io-client expect binary packets to be adjacent to the first string data frame.

Therefore, this issue is fixed with msgpack because :

packets with binary content are sent as one single WebSocket frame (if the WebSocket connection is established).

I'll create a tracking issue for this bug. I think it would be better to separate the custom parser support feature from this bug that should not happen even with the default parser. I'll try to propose a solution as simple as possible for this bug during the week.

Thanks for your great explanations and schemas :).

@Totodore Totodore linked an issue Jan 17, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request socketio-v4 It is only about the version 4 of the socket.io protocol socketio-v5 It is only about the version 5 of the socket.io protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom parser support
3 participants