Skip to content

Make distinction between valid and succeeded pair #4

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 18, 2023
Merged

Make distinction between valid and succeeded pair #4

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
8 changes: 6 additions & 2 deletions lib/candidate_pair.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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
Expand Down
2 changes: 2 additions & 0 deletions lib/conn_check_handler.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
54 changes: 23 additions & 31 deletions lib/controlled_handler.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{} ->
Expand All @@ -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("""
Expand All @@ -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
25 changes: 11 additions & 14 deletions lib/controlling_handler.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{} ->
Expand All @@ -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
Loading