Deduplicate and refactor code

[heartlabs]

Adding complex new features like #64 is made more difficult because of mainly two issues in the code structure.

1. There are two versions of the WebRTC socket and all its components for two compile target (groups) - one for native and one for wasm. All work needs to be done twice and even if both code versions resemble each other in many ways there are also many differences

2. Most of the code lives in one huge file (per target) which is not too easy to understand - the code structure as it is now doesn't quite fit in my head and I find myself looping over and over through the code trying to understand how one thing affects the other and where the data is flowing. For example even after spending hours with it it's still not clear for me what is the main difference between handshake_offer and handshake_accept or the handling of PeerEvent::NewPeer and PeerEvent::Signal and why they are so similar and if the code can be easily deduplicated or not. Or what exactly is the exact purpose of all the different senders and receivers.

Ideally before starting some major refactoring work we can get rid of the separate code for wasm. Unfortunately webrtc-rs doesn't yet support wasm and it's not even planned but they opened an issue for that topic with a help wanted label suggesting that they are open to it:
webrtc-rs/webrtc#351

I would be motivated to have a go at it if you also would be looking forward to use this later in matchbox and if you agree that this could dramatically reduce or even eliminate our wasm-specific code.

Please let me know what you think

[johanhelsing]

Thanks for the feedback! I agree with a lot of it. For some things there are reasons why it is the way it is, other times, it's just sloppiness/laziness/ignorance and there is definitely room for improvement :) I'll try to go through them one by one!

handshake_accept vs handshake_offer

In webrtc there is a concept of one peer initiating (offering) a connection, and another part accepting it. This is just how the protocol works, and we have to work with it.

The part that starts has to call RtcPeerConnection::create_offer after configuring the data channel.

The accepting part has to wait for the offer.

So in a full mesh connection, who should be the offerer and who should be the accepter? Since we don't want races, I just picked the convention that existing peers always initiate (offer) the connection to the newly arriving peers.

Initially, I tried to keep these functions relatively high-level, which is why they both use functions like create_rtc_peer_connection, create_data_channel etc. As we implemented ICE these method became a lot more low-level and a lot less dry.

It would be nice to factor out the common code here into new helper functions/methods.

Events: NewPeer vs Signal

The thought here is that Signals are things the peers themselves create and send to each other, while the other events, currently just NewPeer, are events that the server creates.

From the server's perspective, the signals are just json data it passes on, it makes no attempt to validate them beyond checking that its valid json (maybe it shouldn't even do that). This keeps the signaling server simple-stupid and efficient.

I think I'd like to keep the separation, but it should probably be added as documentation to the enum.

Separate implementations on wasm vs native

Yes, it might seem odd that two things doing essentially the same thing have almost completely separate implementations. My approach to development lately has been to keep it simple-stupid as long as possible, and only create abstractions if there is a lot of duplicated code and the abstraction makes the code easier to understand (or at least not significantly harder).

I have made an attempt to keep the implementations similar, though, with the goal that it should be easier to make similar changes in both implementations. Though this also slipped a little when we implemented ice trickle for wasm.

I think a couple of things make it challenging to share more of the implementation:

• Some of the webrtc-rs API is async when the web-sys counterpart is not
• Much of the implementation is just serializing/deserializing and the target structs here are different. Maybe we could use some sort of generic associated trait for these structs, but I worry we'll just make the code harder to understand by adding another layer of abstraction.
• Much of the implementation is attaching event handlers, here as well the APIs are different with wasm being especially annoying due to how it handles ownership.
• When one API has an annoying aspect, and the other doesn't, it often means the shared implementation would suffer from the worst of both implementations. for instance making things async that doesn't need to be.

That said, I'd be happy to be proven wrong. And I definitely think we should start sharing the implementation where it doesn't make the code harder to understand.

I think a good first step would be to unify how the message loops works wrt ice trickle.

webrtc-rs for wasm and removing the wasm-specific path in this crate

This sounds really interesting. WebRTC-rs is generally more pleasant to work with than wasm. Would be happy to get rid of the separate implementations, provided:

1. Performance is roughly the same
2. Build size is roughly the same (I think webrtc-rs is quite heavy in terms of dependencies :/ )

Things I failed to address etc.

This is getting long, and I hope I don't come across as discouraging or overly defensive. I really appreciate the initiative! :)

In order to start improving things I think it would be good to start small and specific, and have several PRs rather than do everything in one go.

Also, it feels like the overall code quality would be better with more structs and methods and lower number of parameters in functions.

[johanhelsing]

I guess one way to get a feeling for how a shared implementation would look would be to make a shared abstraction for a smaller part, like the data channels themselves for example.

[heartlabs]

Thanks for the lengthy explanations and for sharing your thoughts about separate vs a single implementation! I have an idea how we can get rid of the wasm-specific implementation hopefully without measurable performance degradation and for sure without significantly increasing the build size but it will be a longer project. I will let you know how it goes and if it works like I imagine :)

[simbleau]

I think this is solved, so I'm suggesting we close this. @johanhelsing can have final say.

[heartlabs]

Hi @simbleau,
It's not quite solved the way I imagined it: I wanted to go much more low-level and either add partial wasm support to webrtc-rs or abstract the webrtc functionality we use in a way that we need to duplicate a lot less code. However I didn't get around trying that and don't even know if it's possible or makes sense. Also I probably won't have time to start with it anytime soon.

So it's definitely OK for me as well to close this issue and I will get back with it when I will have any updates.

[johanhelsing]

Agree, if we want to do this, opening a new issue would make it a lot clearer what's actually left to investigate.