Feature: implement EngineIO V3 (closes #22)

[sleeyax]

Here's some initial progress on the V3 implementation. Please provide detailed feedback if you can, I don't consider myself proficient in Rust yet.

Protocol implementation steps:

• Use counting of characters instead of a record separator (\x1e)
• Reverse ping/pong mechanism
• Support non-base64-encoded binary payload

E2E tests: socketio/engine.io-protocol#45

[sleeyax]

Marked this as draft because I still need to implement the rest for the protocol to work properly. I'm moving on with the reverse ping/pong mechanism. Feel free to check the code in the meantime though.

[Totodore]

Thanks for starting all of this !

I have just one suggestion, I think it is much better to have feature flags for versions (with v4,v3), and a default feature flag set on v4 and that you do all your version conditions on those feature flags. It would allow to remove useless code at compile time and also to have the information everywhere in the codebase without having a ProtocolVersion enum.

Also, don't forget to rebase from main regularly

[Totodore]

Nice start, I think the feature flag solution would be better for the codebase.

[Totodore]

I think end to end test-suites are very important, therefore if I have the time I'll try to make one for the v3 version (by copying v4) and modifing things according to changes between v3 & v4. It could be tested with the official engine.io v3 to be sure it is ok.

We would then be sure that this implementation is correct

[sleeyax]

Agreed, feature flags make more sense for this project. I'll make adjustments.

[biryukovmaxim]

Currently ts version has option allowEI3 and it works with both versions.
So I would suggest to have ProtocolVersion as param in case of V3 flag is enabled.

[Totodore]

I agree but it should be a separate feature for when eio3 is already supported.

[sleeyax]

I think end to end test-suites are very important, therefore if I have the time I'll try to make one for the v3 version (by copying v4) and modifing things according to changes between v3 & v4. It could be tested with the official engine.io v3 to be sure it is ok.

We would then be sure that this implementation is correct

Just got started on this as well, feel free to suggest adjustments: socketio/engine.io-protocol#45

[Totodore]

Sure ! I totally agree with you

[sleeyax]

I've addressed everything now :)

[Totodore]

I can't comment on unchanged lines but I think you should rename this part to heartbeat_rx and heartbeat_tx in socket.rs because in v3 it would be ping_rx and ping_tx so it is better to have an agnostic name (and also change comments accordingly) :

    /// Internal channel to receive Pong [`Packets`](Packet) in the heartbeat job
    /// which is running in a separate task
    pong_rx: Mutex<mpsc::Receiver<()>>,
    /// Channel to send Ping [`Packets`](Packet) from the connexion to the heartbeat job
    /// which is running in a separate task
    pub(crate) pong_tx: mpsc::Sender<()>,

[sleeyax]

Round 2 of the PR review : ✔️

Please let me know if more changes are required. I'd be happy to see this merged soon :)

[Totodore]

Perfect, however the action e2e_v3 seems to fail...
I'm going to merge it and maybe we could make another PR to fix if there is still an action error

[biryukovmaxim]

@Totodore oops

socketioxide/engineioxide/src/payload.rs

Line 29 in 3b802d9

match self.reader.read_until(b':', &mut self.buffer) {

there is a security issue:
if there is no separator and a client send a lot of data it will increase buffer until memory is running out.

in case of getting the length it mustn't exceed 8 bytes.
in case of reading until separator(v4) it mustn't exceed configured size.

[Totodore]

Right, let's fix that in another PR