Skip to content

Minor fixes #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Minor fixes #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 11 additions & 34 deletions lib/checklist.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down
141 changes: 83 additions & 58 deletions lib/ice_agent.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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)
Expand All @@ -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("""
Expand Down Expand Up @@ -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}
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions questions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down