Skip to content

Commit ebeedd5

Browse files
mickel8mickel8
committed
Forward data received on a faild pair and re-schedule this pair
1 parent c0d55fa commit ebeedd5

File tree

4 files changed

+205
-50
lines changed

4 files changed

+205
-50
lines changed

‎lib/ex_ice/priv/conn_check_handler/controlled.ex‎

Lines changed: 50 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,14 @@ defmodule ExICE.Priv.ConnCheckHandler.Controlled do
1919
ICEAgent.send_binding_success_response(ice_agent, pair, msg)
2020

2121
%CandidatePair{} = checklist_pair ->
22-
checklist_pair = %CandidatePair{checklist_pair | last_seen: pair.last_seen}
23-
checklist = Map.put(ice_agent.checklist, checklist_pair.id, checklist_pair)
24-
ice_agent = %ICEAgent{ice_agent | checklist: checklist}
22+
checklist_pair =
23+
if checklist_pair.state == :failed do
24+
%CandidatePair{checklist_pair | state: :waiting, last_seen: pair.last_seen}
25+
else
26+
%CandidatePair{checklist_pair | last_seen: pair.last_seen}
27+
end
28+
29+
ice_agent = put_in(ice_agent.checklist[checklist_pair.id], checklist_pair)
2530
ICEAgent.send_binding_success_response(ice_agent, checklist_pair, msg)
2631
end
2732
end
@@ -42,37 +47,51 @@ defmodule ExICE.Priv.ConnCheckHandler.Controlled do
4247
ice_agent = %ICEAgent{ice_agent | checklist: checklist}
4348
ICEAgent.send_binding_success_response(ice_agent, pair, msg)
4449

45-
%CandidatePair{} = checklist_pair ->
46-
if checklist_pair.state == :succeeded do
47-
discovered_pair = Map.fetch!(ice_agent.checklist, checklist_pair.discovered_pair_id)
48-
discovered_pair = %CandidatePair{discovered_pair | last_seen: pair.last_seen}
49-
ice_agent = put_in(ice_agent.checklist[discovered_pair.id], discovered_pair)
50-
51-
if ice_agent.selected_pair_id == nil do
52-
Logger.debug("Nomination request on pair: #{discovered_pair.id}.")
53-
update_nominated_flag(ice_agent, discovered_pair.id, true)
54-
else
55-
ice_agent
56-
end
57-
|> ICEAgent.send_binding_success_response(discovered_pair, msg)
50+
%CandidatePair{state: :succeeded} = checklist_pair ->
51+
discovered_pair = Map.fetch!(ice_agent.checklist, checklist_pair.discovered_pair_id)
52+
discovered_pair = %CandidatePair{discovered_pair | last_seen: pair.last_seen}
53+
ice_agent = put_in(ice_agent.checklist[discovered_pair.id], discovered_pair)
54+
55+
if ice_agent.selected_pair_id == nil do
56+
Logger.debug("Nomination request on pair: #{discovered_pair.id}.")
57+
update_nominated_flag(ice_agent, discovered_pair.id, true)
5858
else
59-
# TODO should we check if this pair is not in failed?
60-
Logger.debug("""
61-
Nomination request on pair that hasn't been verified yet.
62-
We will nominate pair once conn check passes.
63-
Pair: #{inspect(checklist_pair.id)}
64-
""")
59+
ice_agent
60+
end
61+
|> ICEAgent.send_binding_success_response(discovered_pair, msg)
6562

66-
checklist_pair = %CandidatePair{
67-
checklist_pair
68-
| nominate?: true,
69-
last_seen: pair.last_seen
70-
}
63+
%CandidatePair{state: :failed} = checklist_pair ->
64+
Logger.debug("""
65+
Nomination request on failed pair. Re-scheduling pair for conn-check.
66+
We will nominate pair once conn check passes.
67+
Pair: #{inspect(checklist_pair.id)}
68+
""")
7169

72-
checklist = Map.put(ice_agent.checklist, checklist_pair.id, checklist_pair)
73-
ice_agent = %ICEAgent{ice_agent | checklist: checklist}
74-
ICEAgent.send_binding_success_response(ice_agent, checklist_pair, msg)
75-
end
70+
checklist_pair = %CandidatePair{
71+
checklist_pair
72+
| nominate?: true,
73+
last_seen: pair.last_seen,
74+
state: :waiting
75+
}
76+
77+
ice_agent = put_in(ice_agent.checklist[checklist_pair.id], checklist_pair)
78+
ICEAgent.send_binding_success_response(ice_agent, checklist_pair, msg)
79+
80+
%CandidatePair{} = checklist_pair ->
81+
Logger.debug("""
82+
Nomination request on pair that hasn't been verified yet.
83+
We will nominate pair once conn check passes.
84+
Pair: #{inspect(checklist_pair.id)}
85+
""")
86+
87+
checklist_pair = %CandidatePair{
88+
checklist_pair
89+
| nominate?: true,
90+
last_seen: pair.last_seen
91+
}
92+
93+
ice_agent = put_in(ice_agent.checklist[checklist_pair.id], checklist_pair)
94+
ICEAgent.send_binding_success_response(ice_agent, checklist_pair, msg)
7695
end
7796
end
7897

‎lib/ex_ice/priv/conn_check_handler/controlling.ex‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,14 @@ defmodule ExICE.Priv.ConnCheckHandler.Controlling do
3030
ICEAgent.send_binding_success_response(ice_agent, pair, msg)
3131

3232
%CandidatePair{} = checklist_pair ->
33-
checklist_pair = %CandidatePair{checklist_pair | last_seen: pair.last_seen}
34-
checklist = Map.put(ice_agent.checklist, checklist_pair.id, checklist_pair)
35-
ice_agent = %ICEAgent{ice_agent | checklist: checklist}
33+
checklist_pair =
34+
if checklist_pair.state == :failed do
35+
%CandidatePair{checklist_pair | state: :waiting, last_seen: pair.last_seen}
36+
else
37+
%CandidatePair{checklist_pair | last_seen: pair.last_seen}
38+
end
39+
40+
ice_agent = put_in(ice_agent.checklist[checklist_pair.id], checklist_pair)
3641
ICEAgent.send_binding_success_response(ice_agent, checklist_pair, msg)
3742
end
3843
end

‎lib/ex_ice/priv/ice_agent.ex‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -600,20 +600,7 @@ defmodule ExICE.Priv.ICEAgent do
600600
%CandidatePair{} =
601601
pair = Checklist.find_pair(ice_agent.checklist, local_cand.base.id, remote_cand.id)
602602

603-
case pair.state do
604-
:succeeded ->
605-
pair = Map.fetch!(ice_agent.checklist, pair.discovered_pair_id)
606-
handle_data_message(ice_agent, pair, packet)
607-
608-
:failed ->
609-
Logger.error("Received data on failed pair! Ignoring. Pair id: #{pair.id}")
610-
ice_agent
611-
612-
_other ->
613-
# We might receive data on a pair that we haven't checked
614-
# on our side yet.
615-
handle_data_message(ice_agent, pair, packet)
616-
end
603+
handle_data_message(ice_agent, pair, packet)
617604
end
618605
end
619606

@@ -972,7 +959,31 @@ defmodule ExICE.Priv.ICEAgent do
972959
end
973960
end
974961

962+
defp handle_data_message(ice_agent, %{state: :succeeded} = pair, packet) do
963+
# take final pair as local candidate might be srflx
964+
pair = Map.fetch!(ice_agent.checklist, pair.discovered_pair_id)
965+
do_handle_data_message(ice_agent, pair, packet)
966+
end
967+
968+
defp handle_data_message(ice_agent, %{state: :failed} = pair, packet) do
969+
Logger.debug("""
970+
Received data on failed pair. Rescheduling pair for conn check. Pair id: #{pair.id}\
971+
""")
972+
973+
ice_agent = do_handle_data_message(ice_agent, pair, packet)
974+
975+
# re-schedule pair
976+
pair = %{pair | state: :waiting}
977+
ice_agent = put_in(ice_agent.checklist[pair.id], pair)
978+
update_ta_timer(ice_agent)
979+
end
980+
981+
# We might receive data on a pair that we haven't check on our side yet.
975982
defp handle_data_message(ice_agent, pair, packet) do
983+
do_handle_data_message(ice_agent, pair, packet)
984+
end
985+
986+
defp do_handle_data_message(ice_agent, pair, packet) do
976987
pair = %CandidatePair{pair | last_seen: now()}
977988
ice_agent = put_in(ice_agent.checklist[pair.id], pair)
978989

@@ -1135,6 +1146,9 @@ defmodule ExICE.Priv.ICEAgent do
11351146
msg,
11361147
use_cand_attr
11371148
)
1149+
# As a result of handling incoming binding request, we might have re-scheduled pair.
1150+
# Hence, we have to update ta timer.
1151+
|> update_ta_timer()
11381152
else
11391153
{:error, reason}
11401154
when reason in [

‎test/priv/ice_agent_test.exs‎

Lines changed: 119 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ defmodule ExICE.Priv.ICEAgentTest do
22
use ExUnit.Case, async: true
33

44
alias ExICE.Priv.{Candidate, CandidatePair, IfDiscovery, ICEAgent}
5-
alias ExICE.Priv.Attribute.{ICEControlled, ICEControlling, Priority}
5+
alias ExICE.Priv.Attribute.{ICEControlled, ICEControlling, Priority, UseCandidate}
66
alias ExICE.Support.Transport
77

88
alias ExSTUN.Message
@@ -114,7 +114,7 @@ defmodule ExICE.Priv.ICEAgentTest do
114114
end
115115
end
116116

117-
test "don't add pairs with srflx local candidate to the checklist" do
117+
test "doesn't add pairs with srflx local candidate to the checklist" do
118118
ice_agent =
119119
ICEAgent.new(
120120
controlling_process: self(),
@@ -150,6 +150,123 @@ defmodule ExICE.Priv.ICEAgentTest do
150150
assert pair.local_cand_id == host_cand.base.id
151151
end
152152

153+
test "forwards data received on a faild pair and re-schedules" do
154+
remote_cand = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445)
155+
156+
ice_agent =
157+
ICEAgent.new(
158+
controlling_process: self(),
159+
role: :controlling,
160+
transport_module: Transport.Mock,
161+
if_discovery_module: IfDiscovery.Mock
162+
)
163+
|> ICEAgent.set_remote_credentials("someufrag", "somepwd")
164+
|> ICEAgent.gather_candidates()
165+
|> ICEAgent.add_remote_candidate(remote_cand)
166+
167+
[socket] = ice_agent.sockets
168+
169+
# mark pair as failed
170+
[pair] = Map.values(ice_agent.checklist)
171+
ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed})
172+
173+
# clear ta_timer, ignore outgoing binding request that has been generated
174+
ice_agent = ICEAgent.handle_ta_timeout(ice_agent)
175+
assert ice_agent.ta_timer == nil
176+
177+
# feed some data
178+
ice_agent =
179+
ICEAgent.handle_udp(ice_agent, socket, remote_cand.address, remote_cand.port, "some data")
180+
181+
# assert that data has been passed
182+
assert_receive {:ex_ice, _pid, {:data, "some data"}}
183+
184+
# assert that pair is re-scheduled
185+
assert [pair] = Map.values(ice_agent.checklist)
186+
assert pair.state == :waiting
187+
assert ice_agent.ta_timer != nil
188+
end
189+
190+
describe "re-schedules failed pair on incoming binding request" do
191+
test "with controlling ice agent" do
192+
remote_cand = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445)
193+
194+
ice_agent =
195+
ICEAgent.new(
196+
controlling_process: self(),
197+
role: :controlling,
198+
transport_module: Transport.Mock,
199+
if_discovery_module: IfDiscovery.Mock
200+
)
201+
|> ICEAgent.set_remote_credentials("someufrag", "somepwd")
202+
|> ICEAgent.gather_candidates()
203+
|> ICEAgent.add_remote_candidate(remote_cand)
204+
205+
test_rescheduling(ice_agent, remote_cand)
206+
end
207+
208+
test "with controlled ice agent" do
209+
remote_cand = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445)
210+
211+
ice_agent =
212+
ICEAgent.new(
213+
controlling_process: self(),
214+
role: :controlled,
215+
transport_module: Transport.Mock,
216+
if_discovery_module: IfDiscovery.Mock
217+
)
218+
|> ICEAgent.set_remote_credentials("someufrag", "somepwd")
219+
|> ICEAgent.gather_candidates()
220+
|> ICEAgent.add_remote_candidate(remote_cand)
221+
222+
test_rescheduling(ice_agent, remote_cand)
223+
end
224+
225+
defp test_rescheduling(ice_agent, remote_cand) do
226+
[socket] = ice_agent.sockets
227+
228+
# make sure we won't overflow when modifying tiebreakers later on
229+
ice_agent = %{ice_agent | tiebreaker: 100}
230+
231+
# mark pair as failed
232+
[pair] = Map.values(ice_agent.checklist)
233+
ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed})
234+
235+
# clear ta_timer, ignore outgoing binding request that has been generated
236+
ice_agent = ICEAgent.handle_ta_timeout(ice_agent)
237+
assert ice_agent.ta_timer == nil
238+
239+
# feed incoming binding request
240+
ice_attrs =
241+
if ice_agent.role == :controlled do
242+
[%ICEControlling{tiebreaker: ice_agent.tiebreaker + 1}, %UseCandidate{}]
243+
else
244+
[%ICEControlled{tiebreaker: ice_agent.tiebreaker - 1}]
245+
end
246+
247+
attrs =
248+
[
249+
%Username{value: "#{ice_agent.local_ufrag}:someufrag"},
250+
%Priority{priority: 1234}
251+
] ++ ice_attrs
252+
253+
request =
254+
Message.new(%Type{class: :request, method: :binding}, attrs)
255+
|> Message.with_integrity(ice_agent.local_pwd)
256+
|> Message.with_fingerprint()
257+
258+
raw_request = Message.encode(request)
259+
260+
ice_agent =
261+
ICEAgent.handle_udp(ice_agent, socket, remote_cand.address, remote_cand.port, raw_request)
262+
263+
# assert that pair is re-scheduled
264+
assert [pair] = Map.values(ice_agent.checklist)
265+
assert pair.state == :waiting
266+
assert ice_agent.ta_timer != nil
267+
end
268+
end
269+
153270
describe "sends keepalives" do
154271
setup do
155272
remote_cand = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445)

0 commit comments

Comments
 (0)