diff --git a/lib/ex_ice/priv/checklist.ex b/lib/ex_ice/priv/checklist.ex index 54fcc4e..f5cc157 100644 --- a/lib/ex_ice/priv/checklist.ex +++ b/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} diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index a7e214b..27ce8eb 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/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 } @@ -3013,7 +3020,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) @@ -3085,7 +3094,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 diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index 9fb8863..b250401 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -303,7 +303,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 @@ -363,7 +363,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) @@ -454,7 +454,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) @@ -512,8 +512,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 @@ -1245,6 +1244,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 = @@ -1516,7 +1518,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 @@ -1563,6 +1565,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.Mock, + 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.Mock, + 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 @@ -1943,7 +2028,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) @@ -1973,7 +2058,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) @@ -2492,8 +2577,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( @@ -2519,10 +2604,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