Make distinction between valid and succeeded pair

mickel8 · mickel8 · commit 00b3506e2005 · 2023-08-16T13:01:05.000+02:00
diff --git a/lib/candidate_pair.ex b/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
diff --git a/lib/controlled_handler.ex b/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,16 @@ 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 +77,32 @@ defmodule ExICE.ControlledHandler do
     Logger.debug("Nomination succeeded, pair: #{pair_id}")
 
     pair = Map.fetch!(state.checklist, pair_id)
+    succeeded_pair = Map.fetch!(state.checklist, pair.succeeded_pair_id)
 
-    state =
-      cond do
-        state.selected_pair == nil ->
-          Logger.debug("Selecting pair: #{pair_id}")
-          %{state | selected_pair: pair}
+    pair = %CandidatePair{pair | nominate?: false, nominated?: true}
+    succeeded_pair = %CandidatePair{succeeded_pair | nominate?: false}
 
-        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 =
+      state
+      |> put_in([:checklist, pair.id], pair)
+      |> put_in([:checklist, succeeded_pair.id], succeeded_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
diff --git a/lib/controlling_handler.ex b/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,21 @@ 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)
+    succeeded_pair = Map.fetch!(state.checklist, pair.succeeded_pair_id)
+
+    pair = %CandidatePair{pair | nominate?: false, nominated?: true}
+    succeeded_pair = %CandidatePair{succeeded_pair | nominate?: false}
+
+    state =
+      state
+      |> put_in([:checklist, pair.id], pair)
+      |> put_in([:checklist, succeeded_pair.id], succeeded_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)
+    false = Checklist.in_progress?(state.checklist) or Checklist.waiting?(state.checklist)
 
-    %{
-      state
-      | checklist: checklist,
-        nominating?: {false, nil},
-        selected_pair: Map.fetch!(checklist, pair_id)
-    }
+    %{state | nominating?: {false, nil}, selected_pair: pair}
   end
 end
diff --git a/lib/ice_agent.ex b/lib/ice_agent.ex
@@ -136,7 +136,7 @@ defmodule ExICE.ICEAgent do
             stun_server
 
           :error ->
-            Logger.warn("""
+            Logger.warning("""
             Couldn't parse STUN server URI: #{inspect(stun_server)}. \
             Ignoring.\
             """)
@@ -198,7 +198,7 @@ defmodule ExICE.ICEAgent do
         {:set_remote_credentials, ufrag, pwd},
         %{remote_ufrag: ufrag, remote_pwd: pwd} = state
       ) do
-    Logger.warn("Passed the same remote credentials to be set. Ignoring.")
+    Logger.warning("Passed the same remote credentials to be set. Ignoring.")
     {:noreply, state}
   end
 
@@ -212,13 +212,13 @@ defmodule ExICE.ICEAgent do
 
   @impl true
   def handle_cast(:gather_candidates, %{gathering_state: :gathering} = state) do
-    Logger.warn("Can't gather candidates. Gathering already in progress. Ignoring.")
+    Logger.warning("Can't gather candidates. Gathering already in progress. Ignoring.")
     {:noreply, state}
   end
 
   @impl true
   def handle_cast(:gather_candidates, %{gathering_state: :complete} = state) do
-    Logger.warn("Can't gather candidates. ICE restart needed. Ignoring.")
+    Logger.warning("Can't gather candidates. ICE restart needed. Ignoring.")
     {:noreply, state}
   end
 
@@ -265,7 +265,7 @@ defmodule ExICE.ICEAgent do
 
   @impl true
   def handle_cast({:add_remote_candidate, _remote_cand}, %{eoc: true} = state) do
-    Logger.warn("Received remote candidate after end-of-candidates. Ignoring.")
+    Logger.warning("Received remote candidate after end-of-candidates. Ignoring.")
     {:noreply, state}
   end
 
@@ -282,7 +282,7 @@ defmodule ExICE.ICEAgent do
         {:noreply, state}
 
       {:error, reason} ->
-        Logger.warn("Invalid remote candidate, reason: #{inspect(reason)}. Ignoring.")
+        Logger.warning("Invalid remote candidate, reason: #{inspect(reason)}. Ignoring.")
         {:noreply, state}
     end
   end
@@ -320,7 +320,7 @@ defmodule ExICE.ICEAgent do
 
   @impl true
   def handle_cast({:send_data, _data}, %{state: ice_state} = state) do
-    Logger.warn("""
+    Logger.warning("""
     Cannot send data in ICE state: #{inspect(ice_state)}. \
     Data can only be sent in state :connected or :completed. Ignoring.\
     """)
@@ -347,7 +347,7 @@ defmodule ExICE.ICEAgent do
 
   @impl true
   def handle_info(:ta_timeout, state) when state.state in [:completed, :failed] do
-    Logger.warn("""
+    Logger.warning("""
     Ta timer fired in unexpected state: #{state.state}.
     Trying to update gathering and connection states.
     """)
@@ -419,7 +419,7 @@ defmodule ExICE.ICEAgent do
           {:noreply, state}
 
         {:error, reason} ->
-          Logger.warn("Couldn't decode stun message: #{inspect(reason)}")
+          Logger.warning("Couldn't decode stun message: #{inspect(reason)}")
           {:noreply, state}
       end
     else
@@ -430,7 +430,7 @@ defmodule ExICE.ICEAgent do
 
   @impl true
   def handle_info(msg, state) do
-    Logger.warn("Got unexpected msg: #{inspect(msg)}")
+    Logger.warning("Got unexpected msg: #{inspect(msg)}")
     {:noreply, state}
   end
 
@@ -572,15 +572,15 @@ defmodule ExICE.ICEAgent do
         handle_gathering_transaction_response(socket, src_ip, src_port, msg, state)
 
       %Type{class: class, method: :binding} when is_response(class) ->
-        Logger.warn("""
+        Logger.warning("""
         Ignoring binding response with unknown t_id: #{msg.transaction_id}.
         Is it retransmission or we called ICE restart?
         """)
 
         state
 
       other ->
-        Logger.warn("""
+        Logger.warning("""
         Unknown msg from: #{inspect({src_ip, src_port})}, on: #{inspect(socket_addr)}, msg: #{inspect(other)} \
         """)
 
@@ -746,7 +746,7 @@ defmodule ExICE.ICEAgent do
     else
       {:ok, {socket_ip, socket_port}} = :inet.sockname(socket)
 
-      Logger.warn("""
+      Logger.warning("""
       Ignoring conn check response, non-symmetric src and dst addresses.
       Sent from: #{inspect({conn_check_pair.local_cand.base_address, conn_check_pair.local_cand.base_port})}, \
       to: #{inspect({conn_check_pair.remote_cand.address, conn_check_pair.remote_cand.port})}
@@ -906,7 +906,7 @@ defmodule ExICE.ICEAgent do
   # all checklists with the same foundation to Waiting.
   defp add_valid_pair(
          valid_pair,
-         _conn_check_pair,
+         conn_check_pair,
          %CandidatePair{valid?: true} = checklist_pair,
          %{role: :controlling} = state
        )
@@ -915,8 +915,14 @@ defmodule ExICE.ICEAgent do
     # 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 = Map.replace!(state.checklist, checklist_pair.id, checklist_pair)
+
+    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
@@ -931,7 +937,13 @@ defmodule ExICE.ICEAgent do
     resulted from conn check on pair: #{inspect(conn_check_pair.id)}\
     """)
 
-    conn_check_pair = %CandidatePair{conn_check_pair | state: :succeeded, valid?: true}
+    conn_check_pair = %CandidatePair{
+      conn_check_pair
+      | succeeded_pair_id: conn_check_pair.id,
+        discovered_pair_id: conn_check_pair.id,
+        state: :succeeded,
+        valid?: true
+    }
 
     checklist = Map.replace!(state.checklist, conn_check_pair.id, conn_check_pair)
 
@@ -946,8 +958,20 @@ defmodule ExICE.ICEAgent do
     resulted from conn check on pair: #{inspect(conn_check_pair.id)}\
     """)
 
-    conn_check_pair = %CandidatePair{conn_check_pair | state: :succeeded}
-    checklist_pair = %CandidatePair{checklist_pair | state: :succeeded, valid?: true}
+    conn_check_pair = %CandidatePair{
+      conn_check_pair
+      | discovered_pair_id: checklist_pair.id,
+        succeeded_pair_id: conn_check_pair.id,
+        state: :succeeded
+    }
+
+    checklist_pair = %CandidatePair{
+      checklist_pair
+      | discovered_pair_id: checklist_pair.id,
+        succeeded_pair_id: conn_check_pair.id,
+        state: :succeeded,
+        valid?: true
+    }
 
     checklist =
       state.checklist
@@ -967,7 +991,18 @@ defmodule ExICE.ICEAgent do
 
     Logger.debug("New valid pair: #{inspect(valid_pair.id)}")
 
-    conn_check_pair = %CandidatePair{conn_check_pair | state: :succeeded}
+    conn_check_pair = %CandidatePair{
+      conn_check_pair
+      | discovered_pair_id: valid_pair.id,
+        succeeded_pair_id: conn_check_pair.id,
+        state: :succeeded
+    }
+
+    valid_pair = %CandidatePair{
+      valid_pair
+      | discovered_pair_id: valid_pair.id,
+        succeeded_pair_id: conn_check_pair.id
+    }
 
     checklist =
       state.checklist
@@ -1108,6 +1143,10 @@ defmodule ExICE.ICEAgent do
     case Checklist.get_pair_for_nomination(state.checklist) 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 = %{state | nominating?: {true, pair.id}}
+        pair = Map.fetch!(state.checklist, pair.succeeded_pair_id)
         pair = %CandidatePair{pair | state: :waiting, nominate?: true}
         {pair, state} = send_conn_check(pair, state)
         put_in(state, [:checklist, pair.id], pair)
