Fix checklist pruning and adding valid pair (#11)

Author: mickel8

lib/checklist.ex

@@ -9,8 +9,9 @@ defmodule ExICE.Checklist do
 
   @spec get_next_pair(t()) :: CandidatePair.t()
   def get_next_pair(checklist) do
+    # FIXME correctly handle frozen pairs, according to sec 6.1.4.2
     checklist
-    |> Enum.filter(fn {_id, pair} -> pair.state == :waiting end)
+    |> Enum.filter(fn {_id, pair} -> pair.state in [:frozen, :waiting] end)
     |> Enum.max_by(fn {_id, pair} -> pair.priority end, fn -> {nil, nil} end)
     |> elem(1)
   end
@@ -52,7 +53,7 @@ defmodule ExICE.Checklist do
 
   @spec waiting?(t()) :: boolean()
   def waiting?(checklist) do
-    Enum.any?(checklist, fn {_id, pair} -> pair.state == :waiting end)
+    Enum.any?(checklist, fn {_id, pair} -> pair.state in [:frozen, :waiting] end)
   end
 
   @spec in_progress?(t()) :: boolean()
@@ -74,42 +75,18 @@ defmodule ExICE.Checklist do
 
   @spec prune(t()) :: t()
   def prune(checklist) do
-    # We sort the checklist as follows:
-    # * first in flight or done pairs
-    # * next waiting pairs
-    # * in both categories sort by priority
-    # because uniq_by keeps first occurence, we will
-    # always drop newly added pair if the same pair
-    # is already in flight or done.
-    # That's not fully compliant with the RFC 8838
-    # as sec 10 says:
-    #
-    # The agent prunes redundant pairs by following
-    # the rules in Section 6.1.2.4 of [RFC8445] but
-    # checks existing pairs only if they have a state
-    # of Waiting or Frozen;
-    #
-    #
-    # but the code is much easier if we guarantee
-    # there are no duplicate pairs in the checklist.
-    # Also, we should always pick pairs with local
-    # host candidates over local srflx candidates
-    # so it should be okay?
-
-    checklist =
-      checklist
-      |> Enum.sort_by(
-        fn {_id, p} ->
-          pair_state = if p.state in [:waiting, :frozen], do: 0, else: 1
-          {pair_state, p.priority}
-        end,
-        :desc
-      )
+    # This is done according to RFC 8838 sec. 10 
+    {waiting, in_flight_or_done} =
+      Enum.split_with(checklist, fn {_id, p} -> p.state in [:waiting, :frozen] end)
+
+    waiting =
+      waiting
+      |> Enum.sort_by(fn {_id, p} -> p.priority end, :desc)
       |> Enum.uniq_by(fn {_id, p} ->
         {p.local_cand.base_address, p.local_cand.base_port, p.remote_cand}
       end)
 
-    Map.new(checklist)
+    Map.new(waiting ++ in_flight_or_done)
   end
 
   @spec timeout_pairs(t(), [integer()]) :: t()

lib/ice_agent.ex

@@ -842,23 +842,51 @@ defmodule ExICE.ICEAgent do
   defp handle_gathering_transaction_success_response(_socket, _src_ip, _src_port, msg, state) do
     t = Map.fetch!(state.gathering_transactions, msg.transaction_id)
 
-    {:ok, %XORMappedAddress{address: address, port: port}} =
+    {:ok, %XORMappedAddress{address: xor_addr, port: xor_port}} =
       Message.get_attribute(msg, XORMappedAddress)
 
-    c =
-      Candidate.new(
-        :srflx,
-        address,
-        port,
-        t.host_cand.address,
-        t.host_cand.port,
-        t.host_cand.socket
-      )
+    case find_cand(state.local_cands, xor_addr, xor_port) do
+      nil ->
+        c =
+          Candidate.new(
+            :srflx,
+            xor_addr,
+            xor_port,
+            t.host_cand.address,
+            t.host_cand.port,
+            t.host_cand.socket
+          )
+
+        Logger.debug("New srflx candidate: #{inspect(c)}")
+        send(state.controlling_process, {:ex_ice, self(), {:new_candidate, Candidate.marshal(c)}})
+        add_srflx_cand(c, state)
+
+      cand ->
+        Logger.debug("""
+        Not adding srflx candidate as we already have a candidate with the same address.
+        Candidate: #{inspect(cand)}
+        """)
+    end
+    |> update_in([:gathering_transactions, t.t_id], fn t -> %{t | state: :complete} end)
+  end
 
-    Logger.debug("New srflx candidate: #{inspect(c)}")
+  defp handle_gathering_transaction_error_response(_socket, _src_ip, _src_port, msg, state) do
+    t = Map.fetch!(state.gathering_transactions, msg.transaction_id)
 
-    send(state.controlling_process, {:ex_ice, self(), {:new_candidate, Candidate.marshal(c)}})
+    error_code =
+      case Message.get_attribute(msg, ErrorCode) do
+        {:ok, error_code} -> error_code
+        _other -> nil
+      end
+
+    Logger.debug(
+      "Gathering transaction failed, t_id: #{msg.transaction_id}, reason: #{inspect(error_code)}"
+    )
+
+    update_in(state, [:gathering_transactions, t.t_id], fn t -> %{t | state: :failed} end)
+  end
 
+  defp add_srflx_cand(c, state) do
     # replace address and port with candidate base
     # and prune the checklist - see sec. 6.1.2.4
     local_cand = %Candidate{c | address: c.base_address, port: c.base_port}
@@ -884,56 +912,15 @@ defmodule ExICE.ICEAgent do
       Logger.debug("New candidate pairs: #{inspect(added_pairs)}")
     end
 
-    state
-    |> update_in([:local_cands], fn local_cands -> [c | local_cands] end)
-    |> update_in([:gathering_transactions, t.t_id], fn t -> %{t | state: :complete} end)
-    |> update_in([:checklist], fn _ -> checklist end)
-  end
-
-  defp handle_gathering_transaction_error_response(_socket, _src_ip, _src_port, msg, state) do
-    t = Map.fetch!(state.gathering_transactions, msg.transaction_id)
-
-    error_code =
-      case Message.get_attribute(msg, ErrorCode) do
-        {:ok, error_code} -> error_code
-        _other -> nil
-      end
-
-    Logger.debug(
-      "Gathering transaction failed, t_id: #{msg.transaction_id}, reason: #{inspect(error_code)}"
-    )
-
-    update_in(state, [:gathering_transactions, t.t_id], fn t -> %{t | state: :failed} end)
+    %{state | checklist: checklist, local_cands: [c | state.local_cands]}
   end
 
   # Adds valid pair according to sec 7.2.5.3.2
   # TODO sec. 7.2.5.3.3
   # The agent MUST set the states for all other Frozen candidate pairs in
   # all checklists with the same foundation to Waiting.
-  defp add_valid_pair(
-         valid_pair,
-         conn_check_pair,
-         %CandidatePair{valid?: true} = checklist_pair,
-         %{role: :controlling} = state
-       )
-       when are_pairs_equal(valid_pair, checklist_pair) do
-    # valid pair is already in the checklist and it is
-    # marked as valid so this cannot be our first conn check on it -
-    # this means that nominate? flag has to be set
-    true = checklist_pair.nominate?
-    conn_check_pair = %CandidatePair{conn_check_pair | state: :succeeded}
-    checklist_pair = %CandidatePair{checklist_pair | state: :succeeded}
-
-    checklist =
-      state.checklist
-      |> Map.replace!(checklist_pair.id, checklist_pair)
-      |> Map.replace!(conn_check_pair.id, conn_check_pair)
-
-    state = %{state | checklist: checklist}
-    {checklist_pair.id, state}
-  end
-
-  # check against valid_pair == conn_check_pair before
+  #
+  # Check against valid_pair == conn_check_pair before
   # checking against valid_pair == checklist_pair as
   # the second condition is always true if the first one is
   defp add_valid_pair(valid_pair, conn_check_pair, _, state)
@@ -957,6 +944,41 @@ defmodule ExICE.ICEAgent do
     {conn_check_pair.id, state}
   end
 
+  defp add_valid_pair(
+         valid_pair,
+         conn_check_pair,
+         %CandidatePair{valid?: true} = checklist_pair,
+         state
+       )
+       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 =
+      state.checklist
+      |> Map.replace!(checklist_pair.id, checklist_pair)
+      |> Map.replace!(conn_check_pair.id, conn_check_pair)
+
+    state = %{state | checklist: checklist}
+    {checklist_pair.id, state}
+  end
+
   defp add_valid_pair(valid_pair, conn_check_pair, checklist_pair, state)
        when are_pairs_equal(valid_pair, checklist_pair) do
     Logger.debug("""
@@ -1150,7 +1172,7 @@ defmodule ExICE.ICEAgent do
       %CandidatePair{} = pair ->
         Logger.debug("Trying to nominate pair: #{inspect(pair.id)}")
         pair = %CandidatePair{pair | nominate?: true}
-        put_in(state, [:checklist, pair.id], pair)
+        state = put_in(state, [:checklist, pair.id], pair)
         state = %{state | nominating?: {true, pair.id}}
         pair = Map.fetch!(state.checklist, pair.succeeded_pair_id)
         pair = %CandidatePair{pair | state: :waiting, nominate?: true}
@@ -1160,7 +1182,10 @@ defmodule ExICE.ICEAgent do
       nil ->
         # TODO revisit this
         # should we check if state.state == :in_progress?
-        Logger.debug("No pairs for nomination. ICE failed.")
+        Logger.debug("""
+        No pairs for nomination. ICE failed. #{inspect(state.checklist, pretty: true)}
+        """)
+
         change_connection_state(:failed, state)
     end
   end

questions.txt

@@ -5,6 +5,7 @@
 and pruned if redundant - see section 6.1.2.4
 5. Is it possible to connect ice outside docker with ice inside docker?
 6. Is it possible for initial pair state to be different than :waiting when we have only one checklist? 
+Yes, consider scenario where remote peer sends us two the same srflx candidates that differ in ports only. Remote can obtain two srflx candidates when it has multiple local interfaces or has docker bridges on its system. RFC doesnt seem to say we should filter out "redundant" candidates.
 7. Is it possible not to prune some srflx candidate - sec 6.1.2.4?
 8. Is it possible to receive binding response to binding request with USE-CANDIDATE that will result in creating
 a new valid pair? sec 7.2.5.3.4