Don't add pairs with srflx local candidate to the checklist (#52)

Author: mickel8

lib/ex_ice/priv/ice_agent.ex

@@ -15,6 +15,7 @@ defmodule ExICE.Priv.ICEAgent do
   }
 
   alias ExICE.Priv.Attribute.{ICEControlling, ICEControlled, Priority, UseCandidate}
+  alias ExICE.Priv.Candidate.Srflx
 
   alias ExSTUN.Message
   alias ExSTUN.Message.Type
@@ -725,20 +726,19 @@ defmodule ExICE.Priv.ICEAgent do
 
     checklist_foundations = get_foundations(ice_agent)
 
+    # See RFC 8445 sec. 6.1.2.4
+    # "For each pair where the local candidate is reflexive, the candidate
+    # MUST be replaced by its base."
+    # I belive that this is the same as filtering srflx candidates out.
+    # Libnice seems to do the same.
     new_pairs =
-      for local_cand <- local_cands, into: %{} do
-        local_cand =
-          if local_cand.base.type == :srflx do
-            local_cand = put_in(local_cand.base.address, local_cand.base.base_address)
-            put_in(local_cand.base.port, local_cand.base.base_port)
-          else
-            local_cand
-          end
-
+      local_cands
+      |> Enum.reject(fn %mod{} -> mod == Srflx end)
+      |> Map.new(fn local_cand ->
         pair_state = get_pair_state(local_cand, remote_cand, checklist_foundations)
         pair = CandidatePair.new(local_cand, remote_cand, ice_agent.role, pair_state)
         {pair.id, pair}
-      end
+      end)
 
     checklist = Checklist.prune(Map.merge(ice_agent.checklist, new_pairs))
 
@@ -1489,41 +1489,6 @@ defmodule ExICE.Priv.ICEAgent do
     {conn_check_pair.id, ice_agent}
   end
 
-  defp add_valid_pair(
-         ice_agent,
-         valid_pair,
-         conn_check_pair,
-         %CandidatePair{valid?: true} = checklist_pair
-       )
-       when are_pairs_equal(valid_pair, checklist_pair) do
-    Logger.debug("""
-    New valid pair: #{checklist_pair.id} \
-    resulted from conn check on pair: #{conn_check_pair.id} \
-    but there is already such a pair in the checklist marked as valid.
-    Should this ever happen after we don't add redundant srflx candidates?
-    Checklist pair: #{checklist_pair.id}.
-    """)
-
-    # if we get here, don't update discovered_pair_id and succeeded_pair_id of
-    # the checklist pair as they are already set
-    conn_check_pair = %CandidatePair{
-      conn_check_pair
-      | state: :succeeded,
-        succeeded_pair_id: conn_check_pair.id,
-        discovered_pair_id: checklist_pair.id
-    }
-
-    checklist_pair = %CandidatePair{checklist_pair | state: :succeeded}
-
-    checklist =
-      ice_agent.checklist
-      |> Map.replace!(checklist_pair.id, checklist_pair)
-      |> Map.replace!(conn_check_pair.id, conn_check_pair)
-
-    ice_agent = %__MODULE__{ice_agent | checklist: checklist}
-    {checklist_pair.id, ice_agent}
-  end
-
   defp add_valid_pair(ice_agent, valid_pair, conn_check_pair, checklist_pair)
        when are_pairs_equal(valid_pair, checklist_pair) do
     Logger.debug("""

test/priv/ice_agent_test.exs

@@ -114,6 +114,42 @@ defmodule ExICE.Priv.ICEAgentTest do
     end
   end
 
+  test "don't add pairs with srflx local candidate to the checklist" do
+    ice_agent =
+      ICEAgent.new(
+        controlling_process: self(),
+        role: :controlling,
+        transport_module: Transport.Mock,
+        if_discovery_module: IfDiscovery.Mock
+      )
+      |> ICEAgent.set_remote_credentials("someufrag", "somepwd")
+      |> ICEAgent.gather_candidates()
+
+    [socket] = ice_agent.sockets
+    [host_cand] = Map.values(ice_agent.local_cands)
+
+    srflx_cand =
+      ExICE.Priv.Candidate.Srflx.new(
+        address: {192, 168, 0, 2},
+        port: 1234,
+        base_address: host_cand.base.base_address,
+        base_port: host_cand.base.base_port,
+        transport_module: ice_agent.transport_module,
+        socket: socket
+      )
+
+    local_cands = %{host_cand.base.id => host_cand, srflx_cand.base.id => srflx_cand}
+    ice_agent = %{ice_agent | local_cands: local_cands}
+
+    remote_cand = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445)
+
+    ice_agent = ICEAgent.add_remote_candidate(ice_agent, remote_cand)
+
+    # assert there is only one pair with host local candidate
+    assert [pair] = Map.values(ice_agent.checklist)
+    assert pair.local_cand_id == host_cand.base.id
+  end
+
   describe "sends keepalives" do
     setup do
       remote_cand = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445)