Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #89 +/- ##
==========================================
+ Coverage 86.06% 87.13% +1.06%
==========================================
Files 25 27 +2
Lines 1902 2075 +173
==========================================
+ Hits 1637 1808 +171
- Misses 265 267 +2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Karolk99
left a comment
Karolk99
left a comment
There was a problem hiding this comment.
Overall, great job!
- When I read this, I became curious about pion implementation. Did you look at how they manage tcp candidates?
- We should release the ICE and the WebRTC packages before merging this PR.
- We should add some more tests for this feature, if possible. From what I saw in this PR, the only new tests are for candidates' priority and candidates gathering.
| # HACK: using listen sockets here is ugly, but was easier to fit into the existing ICE Agent implementation. | ||
| # This should be changed, especially because we're going to want to close the listen sockets | ||
| # after the connection is successfully established. |
There was a problem hiding this comment.
You can add to this comment that listen_socket is used to access connected_socket.
|
|
||
| @impl true | ||
| def handle_call({:close, listen_socket}, _from, state) do | ||
| # TODO: revisit the closing logic |
There was a problem hiding this comment.
Could you add more specific comment.
| <<length::unsigned-big-16, data::binary-size(length), rest::binary>> -> | ||
| # HACK: this is dirty and means that, with framing, we're miscalculating | ||
| # the bytes_sent and bytes_received counters | ||
| send(state.ice_agent, {:udp, state.listen_socket, src_ip, src_port, data}) |
There was a problem hiding this comment.
We could call {:tcp, ,,,,} then in ICEAgent call Priv.ICEAgent.handle_tcp and only in Priv.ICEAgent add call to handle_udp with comment that this is the same because framing is handled in tcp_client.
There was a problem hiding this comment.
I'm not sure how I feel about this change because ideally (in the future) we'd want the ICE Agent to be the controlling process of the TCP connection, and handle the 3-element {:tcp, socket, packet} messages, not the 5-element {:tcp, listen_socket, ip, port, reconstructed_payload} ones we'd be creating in the Client.
That being said, I won't contest it if you strongly believe this way is better/more descriptive
lib/ex_ice/priv/transport/tcp.ex
Outdated
| def send(listen_socket, dest, packet, tp_opts \\ []) do | ||
| with {:ok, local} <- sockname(listen_socket), | ||
| [{pid, _}] <- Registry.lookup(ExICE.Priv.Registry, local) do | ||
| GenServer.call(pid, {:send, listen_socket, dest, packet, tp_opts}) |
|
Ad 1. No, but I can take a look |
2 participants
The current implementation successfully gathers host and server-reflexive TCP candidates, and connects to the other side. Relay candidates are not supported.
Note that not all STUN servers support TCP (e.g.
stun.l.google.com:19302does not).Potential issues and bottlenecks are outlined in the comments.
ref: elixir-webrtc/ex_webrtc#238
Relevant RFCs: