Don't close candidate on eperm (#85)

Author: Karolk99

.tool-versions

@@ -0,0 +1,2 @@
+erlang 27.3.1
+elixir 1.18.3-otp-27

lib/ex_ice/priv/checklist.ex

@@ -29,9 +29,6 @@ defmodule ExICE.Priv.Checklist do
   def get_valid_pair(checklist) do
     checklist
     |> Stream.map(fn {_id, pair} -> pair end)
-    # pair might have been marked as failed if the associated
-    # local candidate has been closed
-    |> Stream.filter(fn pair -> pair.state == :succeeded end)
     |> Stream.filter(fn pair -> pair.valid? end)
     |> Enum.sort_by(fn pair -> pair.priority end, :desc)
     |> Enum.at(0)
@@ -89,7 +86,7 @@ defmodule ExICE.Priv.Checklist do
   def close_candidate(checklist, local_cand) do
     Enum.reduce(checklist, {[], checklist}, fn {pair_id, pair}, {failed_pair_ids, checklist} ->
       if pair.local_cand_id == local_cand.base.id and pair.state != :failed do
-        checklist = Map.put(checklist, pair_id, %{pair | state: :failed})
+        checklist = Map.put(checklist, pair_id, %{pair | state: :failed, valid?: false})
         {[pair_id | failed_pair_ids], checklist}
       else
         {failed_pair_ids, checklist}

lib/ex_ice/priv/ice_agent.ex

@@ -485,7 +485,9 @@ defmodule ExICE.Priv.ICEAgent do
     Logger.debug("Setting end-of-candidates flag.")
     ice_agent = %{ice_agent | eoc: true}
     # check whether it's time to nominate and if yes, try noimnate
-    maybe_nominate(ice_agent)
+    ice_agent
+    |> maybe_nominate()
+    |> update_connection_state()
   end
 
   @spec send_data(t(), binary()) :: t()
@@ -580,6 +582,7 @@ defmodule ExICE.Priv.ICEAgent do
       |> update_gathering_state()
       |> update_connection_state()
       |> maybe_nominate()
+      |> update_connection_state()
 
     if ice_agent.state in [:completed, :failed] do
       update_ta_timer(ice_agent)
@@ -591,7 +594,9 @@ defmodule ExICE.Priv.ICEAgent do
             ice_agent
 
           {type, tr} ->
-            execute_transaction(ice_agent, type, tr)
+            ice_agent
+            |> execute_transaction(type, tr)
+            |> update_connection_state()
         end
 
       # schedule next check and call update_ta_timer
@@ -1744,6 +1749,7 @@ defmodule ExICE.Priv.ICEAgent do
       conn_check_pair = %CandidatePair{
         conn_check_pair
         | state: :failed,
+          valid?: false,
           non_symmetric_responses_received: conn_check_pair.non_symmetric_responses_received + 1
       }
 
@@ -1817,6 +1823,7 @@ defmodule ExICE.Priv.ICEAgent do
         conn_check_pair = %CandidatePair{
           conn_check_pair
           | state: :failed,
+            valid?: false,
             responses_received: conn_check_pair.responses_received + 1
         }
 
@@ -3045,7 +3052,9 @@ defmodule ExICE.Priv.ICEAgent do
         pair = %CandidatePair{
           pair
           | packets_discarded_on_send: pair.packets_discarded_on_send + 1,
-            bytes_discarded_on_send: pair.bytes_discarded_on_send + byte_size(raw_req)
+            bytes_discarded_on_send: pair.bytes_discarded_on_send + byte_size(raw_req),
+            state: :failed,
+            valid?: false
         }
 
         put_in(ice_agent.checklist[pair.id], pair)
@@ -3117,7 +3126,7 @@ defmodule ExICE.Priv.ICEAgent do
           """)
 
           ice_agent = put_in(ice_agent.local_cands[local_cand.base.id], local_cand)
-          ice_agent = close_candidate(ice_agent, local_cand)
+
           {:error, ice_agent}
         end
     end

test/priv/ice_agent_test.exs

@@ -317,7 +317,7 @@ defmodule ExICE.Priv.ICEAgentTest do
     ice_agent = put_in(ice_agent.local_cands[cand.base.id], cand)
 
     [pair] = Map.values(ice_agent.checklist)
-    pair = %{pair | state: :failed}
+    pair = %{pair | state: :failed, valid?: false}
     ice_agent = put_in(ice_agent.checklist[pair.id], pair)
 
     # try to feed data on closed candidate, it should be ignored
@@ -377,7 +377,7 @@ defmodule ExICE.Priv.ICEAgentTest do
 
     assert ice_agent.state == :closed
     assert ice_agent.gathering_state == :complete
-    assert [%{state: :failed} = pair] = Map.values(ice_agent.checklist)
+    assert [%{state: :failed, valid?: false} = pair] = Map.values(ice_agent.checklist)
     assert [%{base: %{closed?: true}}] = Map.values(ice_agent.local_cands)
     # make sure that sockets and remote cands were not cleared
     assert [_remote_cand] = Map.values(ice_agent.remote_cands)
@@ -468,7 +468,7 @@ defmodule ExICE.Priv.ICEAgentTest do
 
     # mark pair as failed
     [pair] = Map.values(ice_agent.checklist)
-    ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed})
+    ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed, valid?: false})
 
     # clear ta_timer, ignore outgoing binding request that has been generated
     ice_agent = ICEAgent.handle_ta_timeout(ice_agent)
@@ -526,8 +526,7 @@ defmodule ExICE.Priv.ICEAgentTest do
 
       # mark pair as failed
       [pair] = Map.values(ice_agent.checklist)
-      ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed})
-
+      ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed, valid?: false})
       # clear ta_timer, ignore outgoing binding request that has been generated
       ice_agent = ICEAgent.handle_ta_timeout(ice_agent)
       assert ice_agent.ta_timer == nil
@@ -1259,6 +1258,9 @@ defmodule ExICE.Priv.ICEAgentTest do
     end
   end
 
+  @conn_check_byte_size 92
+  @conn_check_with_nomination_byte_size 96
+
   describe "connectivity check" do
     setup do
       ice_agent =
@@ -1592,7 +1594,7 @@ defmodule ExICE.Priv.ICEAgentTest do
           resp
         )
 
-      assert [%CandidatePair{state: :failed}] = Map.values(ice_agent.checklist)
+      assert [%CandidatePair{state: :failed, valid?: false}] = Map.values(ice_agent.checklist)
       assert [new_pair] = Map.values(ice_agent.checklist)
       assert new_pair.state == :failed
       assert new_pair.responses_received == pair.responses_received
@@ -1639,6 +1641,89 @@ defmodule ExICE.Priv.ICEAgentTest do
 
       assert ice_agent.state == :completed
     end
+
+    test "failure on send" do
+      # 1. replace candidate with the mock one that always fails to send data
+      # 2. assert that after unsuccessful conn check sending, ice_agent moves conn pair to the failed state
+
+      ice_agent =
+        ICEAgent.new(
+          controlling_process: self(),
+          role: :controlling,
+          if_discovery_module: IfDiscovery.MockSingle,
+          transport_module: Transport.Mock
+        )
+        |> ICEAgent.set_remote_credentials("someufrag", "somepwd")
+        |> ICEAgent.gather_candidates()
+        |> ICEAgent.add_remote_candidate(@remote_cand)
+        |> ICEAgent.end_of_candidates()
+
+      assert ice_agent.gathering_state == :complete
+
+      # replace candidate with the mock one
+      [local_cand] = Map.values(ice_agent.local_cands)
+      mock_cand = %Candidate.Mock{base: local_cand.base}
+      ice_agent = %{ice_agent | local_cands: %{mock_cand.base.id => mock_cand}}
+
+      # try to send conn check
+      ice_agent = ICEAgent.handle_ta_timeout(ice_agent)
+
+      # assert that the candidate pair has moved to a failed state
+      # and that the state was updated after the packet was discarded
+      assert [
+               %{
+                 state: :failed,
+                 valid?: false,
+                 packets_discarded_on_send: 1,
+                 bytes_discarded_on_send: @conn_check_byte_size
+               }
+             ] = Map.values(ice_agent.checklist)
+
+      assert ice_agent.state == :failed
+    end
+
+    test "failure on send, when nominating" do
+      # 1. make ice agent connected
+      # 2. replace candidate with the mock one that always fails to send data
+      # 3. assert that after unsuccessful nomination sending, ice_agent moves conn pair to the failed state
+
+      ice_agent =
+        ICEAgent.new(
+          controlling_process: self(),
+          role: :controlling,
+          if_discovery_module: IfDiscovery.MockSingle,
+          transport_module: Transport.Mock
+        )
+        |> ICEAgent.set_remote_credentials("someufrag", "somepwd")
+        |> ICEAgent.gather_candidates()
+        |> ICEAgent.add_remote_candidate(@remote_cand)
+
+      assert ice_agent.gathering_state == :complete
+
+      # make ice_agent connected
+      ice_agent = connect(ice_agent)
+
+      # replace candidate with the mock one
+      [local_cand] = Map.values(ice_agent.local_cands)
+      mock_cand = %Candidate.Mock{base: local_cand.base}
+      ice_agent = %{ice_agent | local_cands: %{mock_cand.base.id => mock_cand}}
+
+      # trigger pair nomination
+      ice_agent = ICEAgent.end_of_candidates(ice_agent)
+
+      # assert that the candidate pair has moved to a failed state
+      # and that the state was updated after the packet was discarded
+      assert [
+               %{
+                 state: :failed,
+                 valid?: false,
+                 packets_discarded_on_send: 1,
+                 bytes_discarded_on_send: @conn_check_with_nomination_byte_size
+               }
+             ] = Map.values(ice_agent.checklist)
+
+      assert ice_agent.state == :failed
+    end
   end
 
   describe "connectivity check with aggressive nomination" do
@@ -2019,7 +2104,7 @@ defmodule ExICE.Priv.ICEAgentTest do
     ice_agent = ICEAgent.handle_pair_timeout(ice_agent)
 
     # assert that the pair is marked as failed
-    assert [%CandidatePair{state: :failed}] = Map.values(ice_agent.checklist)
+    assert [%CandidatePair{state: :failed, valid?: false}] = Map.values(ice_agent.checklist)
 
     # trigger eoc timeout
     ice_agent = ICEAgent.handle_eoc_timeout(ice_agent)
@@ -2049,7 +2134,7 @@ defmodule ExICE.Priv.ICEAgentTest do
 
     # mark pair as failed
     [pair] = Map.values(ice_agent.checklist)
-    ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed})
+    ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed, valid?: false})
 
     # set eoc flag
     failed_ice_agent = ICEAgent.end_of_candidates(ice_agent)
@@ -2568,8 +2653,8 @@ defmodule ExICE.Priv.ICEAgentTest do
   test "candidate fails to send data when ice is connected" do
     # 1. make ice agent connected
     # 2. replace candidate with the mock one that always fails to send  data
-    # 3. assert that after unsuccessful data sending, ice_agent moves to the failed state
-    # as there are no other pairs
+    # 3. assert that after unsuccessful data sending, ice_agent doesn't move to the failed state
+    # even when there is only one pair
 
     ice_agent =
       ICEAgent.new(
@@ -2595,10 +2680,18 @@ defmodule ExICE.Priv.ICEAgentTest do
     # try to send some data
     ice_agent = ICEAgent.send_data(ice_agent, "somedata")
 
-    # assert that local cand has been closed and the agent moved to the failed state
-    assert [%{base: %{closed?: true}}] = Map.values(ice_agent.local_cands)
-    assert ice_agent.state == :failed
-    assert [%{state: :failed}] = Map.values(ice_agent.checklist)
+    # assert that the local candidate hasn't been closed and that the agent hasn't moved to a failed state
+    assert [%{base: %{closed?: false}}] = Map.values(ice_agent.local_cands)
+    assert ice_agent.state == :connected
+
+    # assert that the local candidate's state was updated after the packet was discarded
+    assert [
+             %{
+               state: :succeeded,
+               packets_discarded_on_send: 1,
+               bytes_discarded_on_send: 8
+             }
+           ] = Map.values(ice_agent.checklist)
   end
 
   test "relay connection" do