Add set_role/2 (#72)

Author: mickel8

lib/ex_ice/ice_agent.ex

@@ -52,6 +52,9 @@ defmodule ExICE.ICEAgent do
   All notifications are by default sent to a process that spawns `ExICE`.
   This behavior can be overwritten using the following options.
 
+  * `role` - agent's role. If not set, it can be later choosen with `set_role/2`. Please note, that
+    until role is set, adding remote candidates or gathering local candidates won't possible, and calls to these
+    functions will be ignored. Defaults to `nil`.
   * `ip_filter` - filter applied when gathering host candidates
   * `ports` - ports that will be used when gathering host candidates, otherwise the ports are chosen by the OS
   * `ice_servers` - list of STUN/TURN servers
@@ -64,6 +67,7 @@ defmodule ExICE.ICEAgent do
   * `on_new_candidate` - where to send new candidates. Defaults to a process that spawns `ExICE`.
   """
   @type opts() :: [
+          role: role() | nil,
           ip_filter: ip_filter(),
           ports: Enumerable.t(non_neg_integer()),
           ice_servers: [
@@ -86,9 +90,9 @@ defmodule ExICE.ICEAgent do
   Process calling this function is called a `controlling process` and
   has to be prepared for receiving ExICE messages described by `t:signal/0`.
   """
-  @spec start_link(role(), opts()) :: GenServer.on_start()
-  def start_link(role, opts \\ []) do
-    GenServer.start_link(__MODULE__, opts ++ [role: role, controlling_process: self()])
+  @spec start_link(opts()) :: GenServer.on_start()
+  def start_link(opts \\ []) when is_list(opts) do
+    GenServer.start_link(__MODULE__, opts ++ [controlling_process: self()])
   end
 
   @doc """
@@ -123,6 +127,14 @@ defmodule ExICE.ICEAgent do
     GenServer.call(ice_agent, {:on_new_candidate, send_to})
   end
 
+  @doc """
+  Gets agent's role.
+  """
+  @spec get_role(pid()) :: ExICE.Agent.t() | nil
+  def get_role(ice_agent) do
+    GenServer.call(ice_agent, :get_role)
+  end
+
   @doc """
   Gets local credentials.
 
@@ -152,6 +164,20 @@ defmodule ExICE.ICEAgent do
     GenServer.call(ice_agent, :get_remote_candidates)
   end
 
+  @doc """
+  Sets agent's role.
+
+  In case of WebRTC, agent's role depends on who sends the first offer.
+  Since an agent has to be initialized at the very beginning, there is no
+  possibility to set its role in the constructor.
+
+  This function can only be called once. Subsequent calls will be ignored.
+  """
+  @spec set_role(pid(), role()) :: :ok
+  def set_role(ice_agent, role) do
+    GenServer.cast(ice_agent, {:set_role, role})
+  end
+
   @doc """
   Sets remote credentials.
 
@@ -283,6 +309,12 @@ defmodule ExICE.ICEAgent do
     {:reply, :ok, %{state | ice_agent: ice_agent}}
   end
 
+  @impl true
+  def handle_call(:get_role, _from, state) do
+    role = ExICE.Priv.ICEAgent.get_role(state.ice_agent)
+    {:reply, role, state}
+  end
+
   @impl true
   def handle_call(:get_local_credentials, _from, state) do
     {local_ufrag, local_pwd} = ExICE.Priv.ICEAgent.get_local_credentials(state.ice_agent)
@@ -307,6 +339,12 @@ defmodule ExICE.ICEAgent do
     {:reply, stats, state}
   end
 
+  @impl true
+  def handle_cast({:set_role, role}, state) do
+    ice_agent = ExICE.Priv.ICEAgent.set_role(state.ice_agent, role)
+    {:noreply, %{state | ice_agent: ice_agent}}
+  end
+
   @impl true
   def handle_cast({:set_remote_credentials, ufrag, pwd}, state) do
     ice_agent = ExICE.Priv.ICEAgent.set_remote_credentials(state.ice_agent, ufrag, pwd)

lib/ex_ice/priv/ice_agent.ex

@@ -153,7 +153,7 @@ defmodule ExICE.Priv.ICEAgent do
       transport_module: transport_module,
       gatherer: Gatherer.new(if_discovery_module, transport_module, ip_filter, ports),
       ice_transport_policy: opts[:ice_transport_policy] || :all,
-      role: Keyword.fetch!(opts, :role),
+      role: opts[:role],
       tiebreaker: generate_tiebreaker(),
       local_ufrag: local_ufrag,
       local_pwd: local_pwd,
@@ -182,6 +182,9 @@ defmodule ExICE.Priv.ICEAgent do
     %__MODULE__{ice_agent | on_new_candidate: send_to}
   end
 
+  @spec get_role(t()) :: ExICE.Agent.role() | nil
+  def get_role(ice_agent), do: ice_agent.role
+
   @spec get_local_credentials(t()) :: {binary(), binary()}
   def get_local_credentials(ice_agent) do
     {ice_agent.local_ufrag, ice_agent.local_pwd}
@@ -227,6 +230,16 @@ defmodule ExICE.Priv.ICEAgent do
     }
   end
 
+  @spec set_role(t(), ExICE.ICEAgent.role()) :: t()
+  def set_role(%__MODULE__{role: nil} = ice_agent, role) do
+    %__MODULE__{ice_agent | role: role}
+  end
+
+  def set_role(%__MODULE__{} = ice_agent, _role) do
+    Logger.warning("Can't set role. Role already set. Ignoring.")
+    ice_agent
+  end
+
   @spec set_remote_credentials(t(), binary(), binary()) :: t()
   def set_remote_credentials(%__MODULE__{state: :failed} = ice_agent, _, _) do
     Logger.debug("Tried to set remote credentials in failed state. ICE restart needed. Ignoring.")
@@ -269,6 +282,11 @@ defmodule ExICE.Priv.ICEAgent do
     ice_agent
   end
 
+  def gather_candidates(%__MODULE__{role: nil} = ice_agent) do
+    Logger.warning("Can't gather candidates without role. Set the role with `set_role/2`.")
+    ice_agent
+  end
+
   def gather_candidates(%__MODULE__{gathering_state: :gathering} = ice_agent) do
     Logger.warning("Can't gather candidates. Gathering already in progress. Ignoring.")
     ice_agent
@@ -341,19 +359,30 @@ defmodule ExICE.Priv.ICEAgent do
 
   def add_remote_candidate(%__MODULE__{eoc: true} = ice_agent, remote_cand) do
     Logger.warning("""
-    Received remote candidate after end-of-candidates. Ignoring.
+    Can't add remote candidate after end-of-candidates. Ignoring. \
     Candidate: #{inspect(remote_cand)}\
     """)
 
     ice_agent
   end
 
+  def add_remote_candidate(%__MODULE__{role: nil} = ice_agent, remote_cand) do
+    Logger.warning("""
+    Can't add remote candidate without role. \
+    Set the role with `set_role/2`. Ignoring. \
+    Candidate: #{inspect(remote_cand)}
+    """)
+
+    ice_agent
+  end
+
   def add_remote_candidate(
         %__MODULE__{remote_ufrag: nil, remote_pwd: nil} = ice_agent,
         remote_cand
       ) do
     Logger.warning("""
-    Received remote candidate but there are no remote credentials. Ignoring.
+    Can't add remote candidate without remote credentials. \
+    Set remote credentials with `set_remote_credentials/3`. Ignoring. \
     Candidate: #{inspect(remote_cand)}\
     """)
 

test/ice_agent_test.exs

@@ -3,8 +3,20 @@ defmodule ExICE.ICEAgentTest do
 
   alias ExICE.ICEAgent
 
+  test "get_role/1" do
+    {:ok, agent} = ICEAgent.start_link(role: :controlling)
+    assert ICEAgent.get_role(agent) == :controlling
+  end
+
+  test "set_role/2" do
+    {:ok, agent} = ICEAgent.start_link()
+    assert ICEAgent.get_role(agent) == nil
+    assert :ok == ICEAgent.set_role(agent, :controlling)
+    assert ICEAgent.get_role(agent) == :controlling
+  end
+
   test "gather_candidates/1" do
-    {:ok, agent} = ICEAgent.start_link(:controlling)
+    {:ok, agent} = ICEAgent.start_link(role: :controlling)
     :ok = ICEAgent.gather_candidates(agent)
 
     assert_receive {:ex_ice, ^agent, {:gathering_state_change, :gathering}}
@@ -19,7 +31,7 @@ defmodule ExICE.ICEAgentTest do
   end
 
   test "get_stats/1" do
-    {:ok, agent} = ICEAgent.start_link(:controlling)
+    {:ok, agent} = ICEAgent.start_link(role: :controlling)
 
     assert %{
              bytes_sent: 0,

test/integration/p2p_test.exs

@@ -17,9 +17,9 @@ defmodule ExICE.Integration.P2PTest do
     end
 
     {:ok, agent1} =
-      ICEAgent.start_link(:controlling, ip_filter: ip_filter, ice_servers: ice_servers)
+      ICEAgent.start_link(role: :controlling, ip_filter: ip_filter, ice_servers: ice_servers)
 
-    {:ok, agent2} = ICEAgent.start_link(:controlled, ip_filter: ip_filter, ice_servers: [])
+    {:ok, agent2} = ICEAgent.start_link(role: :controlled, ip_filter: ip_filter, ice_servers: [])
 
     {:ok, a1_ufrag, a1_pwd} = ICEAgent.get_local_credentials(agent1)
     {:ok, a2_ufrag, a2_pwd} = ICEAgent.get_local_credentials(agent2)
@@ -94,10 +94,10 @@ defmodule ExICE.Integration.P2PTest do
     end
 
     {:ok, agent1} =
-      ICEAgent.start_link(:controlled, ip_filter: ip_filter, ice_servers: ice_servers)
+      ICEAgent.start_link(role: :controlled, ip_filter: ip_filter, ice_servers: ice_servers)
 
     {:ok, agent2} =
-      ICEAgent.start_link(:controlled, ip_filter: ip_filter, ice_servers: ice_servers)
+      ICEAgent.start_link(role: :controlled, ip_filter: ip_filter, ice_servers: ice_servers)
 
     {:ok, a1_ufrag, a1_pwd} = ICEAgent.get_local_credentials(agent1)
     {:ok, a2_ufrag, a2_pwd} = ICEAgent.get_local_credentials(agent2)
@@ -144,14 +144,15 @@ defmodule ExICE.Integration.P2PTest do
     end
 
     {:ok, agent1} =
-      ICEAgent.start_link(:controlling,
+      ICEAgent.start_link(
+        role: :controlling,
         ip_filter: ip_filter,
         ice_servers: ice_servers,
         ice_transport_policy: :relay
       )
 
     {:ok, agent2} =
-      ICEAgent.start_link(:controlled, ip_filter: ip_filter, ice_servers: [])
+      ICEAgent.start_link(role: :controlled, ip_filter: ip_filter, ice_servers: [])
 
     {:ok, a1_ufrag, a1_pwd} = ICEAgent.get_local_credentials(agent1)
     {:ok, a2_ufrag, a2_pwd} = ICEAgent.get_local_credentials(agent2)

test/priv/ice_agent_test.exs

@@ -143,36 +143,75 @@ defmodule ExICE.Priv.ICEAgentTest do
       assert %{} == ice_agent.remote_cands
     end
 
+    test "without role", %{ice_agent: ice_agent} do
+      ice_agent = %{ice_agent | role: nil}
+      ice_agent = ICEAgent.add_remote_candidate(ice_agent, @remote_cand)
+      assert %{} == ice_agent.remote_cands
+    end
+
     test "after setting end-of-candidates", %{ice_agent: ice_agent} do
       ice_agent = ICEAgent.end_of_candidates(ice_agent)
       ice_agent = ICEAgent.add_remote_candidate(ice_agent, @remote_cand)
       assert %{} == ice_agent.remote_cands
     end
   end
 
-  test "gather_candidates/1 creates pairs if there already are any remote candidates" do
+  test "set_role/2" do
     ice_agent =
       ICEAgent.new(
         controlling_process: self(),
-        role: :controlling,
+        role: nil,
         if_discovery_module: IfDiscovery.Mock,
         transport_module: Transport.Mock
       )
       |> ICEAgent.set_remote_credentials("remoteufrag", "remotepwd")
 
-    ice_agent = ICEAgent.add_remote_candidate(ice_agent, @remote_cand)
+    ice_agent = ICEAgent.set_role(ice_agent, :controlling)
+    assert ice_agent.role == :controlling
 
-    # assert that there are no pairs and no local cands
-    assert [] == Map.values(ice_agent.checklist)
-    assert [] == Map.values(ice_agent.local_cands)
+    # role shouldn't change as we don't allow for changing the role once it has been initialized
+    ice_agent = ICEAgent.set_role(ice_agent, :controlled)
+    assert ice_agent.role == :controlling
 
-    # gather candidates
-    ice_agent = ICEAgent.gather_candidates(ice_agent)
-    [host_cand] = Map.values(ice_agent.local_cands)
+    # assert that timer has not been fired as there is no work to do
+    refute_receive :ta_timeout
+  end
+
+  describe "gather_candidates/1" do
+    setup do
+      ice_agent =
+        ICEAgent.new(
+          controlling_process: self(),
+          role: :controlling,
+          if_discovery_module: IfDiscovery.Mock,
+          transport_module: Transport.Mock
+        )
+        |> ICEAgent.set_remote_credentials("remoteufrag", "remotepwd")
+
+      %{ice_agent: ice_agent}
+    end
+
+    test "when there already are remote candidates", %{ice_agent: ice_agent} do
+      ice_agent = ICEAgent.add_remote_candidate(ice_agent, @remote_cand)
+
+      # assert that there are no pairs and no local cands
+      assert [] == Map.values(ice_agent.checklist)
+      assert [] == Map.values(ice_agent.local_cands)
 
-    # assert that a new pair has been created
-    assert [%CandidatePair{} = cand_pair] = Map.values(ice_agent.checklist)
-    assert cand_pair.local_cand_id == host_cand.base.id
+      # gather candidates
+      ice_agent = ICEAgent.gather_candidates(ice_agent)
+      [host_cand] = Map.values(ice_agent.local_cands)
+
+      # assert that a new pair has been created
+      assert [%CandidatePair{} = cand_pair] = Map.values(ice_agent.checklist)
+      assert cand_pair.local_cand_id == host_cand.base.id
+    end
+
+    test "without role", %{ice_agent: ice_agent} do
+      ice_agent = %{ice_agent | role: nil}
+      ice_agent = ICEAgent.gather_candidates(ice_agent)
+      assert %{} == ice_agent.local_cands
+    end
   end
 
   test "handle_udp/5" do