Make distinction between valid and succeeded pair (#4)

Author: mickel8

lib/candidate_pair.ex

@@ -17,7 +17,9 @@ defmodule ExICE.CandidatePair do
           priority: non_neg_integer(),
           remote_cand: Candidate.t(),
           state: state(),
-          valid?: boolean
+          valid?: boolean,
+          succeeded_pair_id: integer() | nil,
+          discovered_pair_id: integer() | nil
         }
 
   @enforce_keys [:id, :local_cand, :remote_cand, :priority]
@@ -26,7 +28,9 @@ defmodule ExICE.CandidatePair do
                 nominate?: false,
                 nominated?: false,
                 state: :frozen,
-                valid?: false
+                valid?: false,
+                succeeded_pair_id: nil,
+                discovered_pair_id: nil
               ]
 
   @doc false

lib/conn_check_handler.ex

@@ -20,6 +20,8 @@ defmodule ExICE.ConnCheckHandler do
 
   Its implementation is based on sec. 7.2.5.3.4.
 
+  `pair_id` is the id of valid pair, not the succeeded one.
+
   The meaning of `nominate?` flag depends on the ice agent role.
 
   In case of controlled, it means that either we received nomination

lib/controlled_handler.ex

@@ -18,9 +18,10 @@ defmodule ExICE.ControlledHandler do
         Logger.debug("Adding new candidate pair: #{inspect(pair)}")
         put_in(state, [:checklist, pair.id], pair)
 
-      %CandidatePair{} = pair when pair == state.selected_pair ->
+      %CandidatePair{} = pair
+      when state.selected_pair != nil and pair.discovered_pair_id == state.selected_pair.id ->
         # to be honest this might also be a retransmission
-        Logger.debug("Keepalive on selected pair: #{pair.id}")
+        Logger.debug("Keepalive on selected pair: #{pair.discovered_pair_id}")
         state
 
       %CandidatePair{} ->
@@ -44,19 +45,15 @@ defmodule ExICE.ControlledHandler do
         pair = %CandidatePair{pair | nominate?: true}
         put_in(state, [:checklist, pair.id], pair)
 
-      %CandidatePair{} = pair when pair == state.selected_pair ->
+      %CandidatePair{} = pair
+      when state.selected_pair != nil and pair.discovered_pair_id == state.selected_pair.id ->
         Logger.debug("Keepalive on selected pair: #{pair.id}")
         state
 
       %CandidatePair{} = pair ->
         if pair.state == :succeeded do
-          # FIXME we should check against valid? flag
-          # it's possible to have to pairs: one with state succeeded
-          # and flag valid? set to false, and the other one with state
-          # succeeded and flag valid? set to true
-          # TODO should we call this selected or nominated pair
-          Logger.debug("Nomination request on valid pair: #{pair.id}.")
-          update_nominated_flag(state, pair.id, true)
+          Logger.debug("Nomination request on pair: #{pair.id}.")
+          update_nominated_flag(state, pair.discovered_pair_id, true)
         else
           # TODO should we check if this pair is not in failed?
           Logger.debug("""
@@ -79,31 +76,26 @@ defmodule ExICE.ControlledHandler do
     Logger.debug("Nomination succeeded, pair: #{pair_id}")
 
     pair = Map.fetch!(state.checklist, pair_id)
+    pair = %CandidatePair{pair | nominate?: false, nominated?: true}
 
-    state =
-      cond do
-        state.selected_pair == nil ->
-          Logger.debug("Selecting pair: #{pair_id}")
-          %{state | selected_pair: pair}
-
-        state.selected_pair != nil and pair.priority >= state.selected_pair.priority ->
-          Logger.debug("""
-          Selecting new pair with higher priority. \
-          New pair: #{pair_id}, old pair: #{state.selected_pair.id}.\
-          """)
+    state = put_in(state, [:checklist, pair.id], pair)
 
-          %{state | selected_pair: pair}
+    cond do
+      state.selected_pair == nil ->
+        Logger.debug("Selecting pair: #{pair_id}")
+        %{state | selected_pair: pair}
 
-        true ->
-          Logger.debug("Not selecting a new pair as it has lower priority")
-          state
-      end
+      state.selected_pair != nil and pair.priority >= state.selected_pair.priority ->
+        Logger.debug("""
+        Selecting new pair with higher priority. \
+        New pair: #{pair_id}, old pair: #{state.selected_pair.id}.\
+        """)
 
-    checklist =
-      Map.update!(state.checklist, pair_id, fn pair ->
-        %CandidatePair{pair | nominate?: false, nominated?: true}
-      end)
+        %{state | selected_pair: pair}
 
-    %{state | checklist: checklist, selected_pair: Map.fetch!(checklist, pair_id)}
+      true ->
+        Logger.debug("Not selecting a new pair as it has lower priority")
+        state
+    end
   end
 end

lib/controlling_handler.ex

@@ -29,9 +29,10 @@ defmodule ExICE.ControllingHandler do
         Logger.debug("Adding new candidate pair: #{inspect(pair)}")
         put_in(state, [:checklist, pair.id], pair)
 
-      %CandidatePair{} = pair when pair == state.selected_pair ->
+      %CandidatePair{} = pair
+      when state.selected_pair != nil and pair.discovered_pair_id == state.selected_pair.id ->
         # to be honest this might also be a retransmission
-        Logger.debug("Keepalive on selected pair: #{pair.id}")
+        Logger.debug("Keepalive on selected pair: #{pair.discovered_pair_id}")
         state
 
       %CandidatePair{} ->
@@ -48,21 +49,17 @@ defmodule ExICE.ControllingHandler do
     Logger.debug("Nomination succeeded. Selecting pair: #{inspect(pair_id)}")
     state = ICEAgent.change_connection_state(:completed, state)
 
-    checklist =
-      Map.update!(state.checklist, pair_id, fn pair ->
-        %CandidatePair{pair | nominate?: false, nominated?: true}
-      end)
+    pair = Map.fetch!(state.checklist, pair_id)
+    pair = %CandidatePair{pair | nominate?: false, nominated?: true}
+
+    state = put_in(state, [:checklist, pair.id], pair)
 
     # the controlling agent could nominate only when eoc was set
     # and checklist finished
-    # important: we have to check on a new checklist
-    false = Checklist.in_progress?(checklist) or Checklist.waiting?(checklist)
+    unless Checklist.finished?(state.checklist) do
+      Logger.warning("Nomination succeeded but checklist hasn't finished.")
+    end
 
-    %{
-      state
-      | checklist: checklist,
-        nominating?: {false, nil},
-        selected_pair: Map.fetch!(checklist, pair_id)
-    }
+    %{state | nominating?: {false, nil}, selected_pair: pair}
   end
 end