Don't move to the closed state after receiving close_notify alert

Author: mickel8

lib/ex_webrtc/dtls_transport.ex

@@ -131,8 +131,8 @@ defmodule ExWebRTC.DTLSTransport do
       remote_cert: nil,
       remote_base64_cert: nil,
       remote_fingerprint: nil,
-      in_srtp: ExLibSRTP.new(),
-      out_srtp: ExLibSRTP.new(),
+      in_srtp: nil,
+      out_srtp: nil,
       # sha256 hex dump
       peer_fingerprint: nil,
       dtls_state: :new,
@@ -217,9 +217,18 @@ defmodule ExWebRTC.DTLSTransport do
   end
 
   @impl true
-  def handle_call({:start_dtls, mode, peer_fingerprint}, _from, %{dtls: nil} = state)
-      when mode in [:active, :passive] do
-    Logger.debug("Started DTLSTransport with role #{mode}")
+  def handle_call(
+        {:start_dtls, mode, peer_fingerprint},
+        _from,
+        %{dtls: dtls, dtls_state: dtls_state} = state
+      )
+      when mode in [:active, :passive] and (dtls == nil or dtls_state == :closed) do
+    if dtls_state == :closed do
+      Logger.debug("Re-initializing DTLSTransport with role #{mode}")
+    else
+      Logger.debug("Starting DTLSTransport with role #{mode}")
+    end
+
     ex_dtls_mode = if mode == :active, do: :client, else: :server
 
     dtls =
@@ -241,7 +250,10 @@ defmodule ExWebRTC.DTLSTransport do
     state = %{
       state
       | dtls: dtls,
+        dtls_state: :new,
         mode: mode,
+        in_srtp: ExLibSRTP.new(),
+        out_srtp: ExLibSRTP.new(),
         peer_fingerprint: peer_fingerprint,
         buffered_remote_packets: nil
     }

lib/ex_webrtc/peer_connection.ex

@@ -1388,8 +1388,22 @@ defmodule ExWebRTC.PeerConnection do
   @impl true
   def handle_call(:close, _from, state) do
     Logger.debug("Closing peer connection")
-    # don't emit state change events
-    state = do_close(state, notify: false)
+    transceivers = Enum.map(state.transceivers, &RTPTransceiver.stop(&1))
+    sctp_transport = SCTPTransport.close_abruptly(state.sctp_transport)
+    :ok = DTLSTransport.close(state.dtls_transport)
+    :ok = state.ice_transport.close(state.ice_pid)
+
+    state =
+      %{
+        state
+        | ice_state: :closed,
+          dtls_state: :closed,
+          transceivers: transceivers,
+          sctp_transport: sctp_transport
+      }
+      |> update_signaling_state(:closed, notify: false)
+      |> update_conn_state(:closed, notify: false)
+
     {:reply, :ok, state}
   end
 
@@ -1548,17 +1562,12 @@ defmodule ExWebRTC.PeerConnection do
 
     next_conn_state = next_conn_state(state.ice_state, new_dtls_state)
 
-    if next_conn_state == :closed and state.conn_state != :closed do
-      state = do_close(state)
-      {:noreply, state}
-    else
-      state =
-        %{state | dtls_state: new_dtls_state}
-        |> update_conn_state(next_conn_state)
-        |> maybe_connect_sctp()
+    state =
+      %{state | dtls_state: new_dtls_state}
+      |> update_conn_state(next_conn_state)
+      |> maybe_connect_sctp()
 
-      {:noreply, state}
-    end
+    {:noreply, state}
   end
 
   @impl true
@@ -2330,8 +2339,6 @@ defmodule ExWebRTC.PeerConnection do
   # Closed connection can't be reused.
   defp next_conn_state(:closed, _dtls_state), do: :closed
 
-  defp next_conn_state(_ice_state, :closed), do: :closed
-
   defp next_conn_state(:failed, _dtls_state), do: :failed
 
   defp next_conn_state(_ice_state, :failed), do: :failed
@@ -2342,8 +2349,9 @@ defmodule ExWebRTC.PeerConnection do
        when ice_state in [:new, :checking] or dtls_state in [:new, :connecting],
        do: :connecting
 
-  defp next_conn_state(ice_state, :connected) when ice_state in [:connected, :completed],
-    do: :connected
+  defp next_conn_state(ice_state, dtls_state)
+       when ice_state in [:connected, :completed] and dtls_state in [:connected, :closed],
+       do: :connected
 
   defp update_conn_state(state, conn_state, opts \\ [])
   defp update_conn_state(%{conn_state: conn_state} = state, conn_state, _opts), do: state
@@ -2551,27 +2559,6 @@ defmodule ExWebRTC.PeerConnection do
     %SessionDescription{type: type, sdp: to_string(sdp)}
   end
 
-  defp do_close(state, opts \\ []) do
-    transceivers = Enum.map(state.transceivers, &RTPTransceiver.stop(&1))
-    sctp_transport = SCTPTransport.close_abruptly(state.sctp_transport)
-    :ok = DTLSTransport.close(state.dtls_transport)
-    :ok = state.ice_transport.close(state.ice_pid)
-
-    if opts[:notify] != false do
-      notify(state.owner, {:ice_connection_state_change, :closed})
-    end
-
-    %{
-      state
-      | ice_state: :closed,
-        dtls_state: :closed,
-        transceivers: transceivers,
-        sctp_transport: sctp_transport
-    }
-    |> update_signaling_state(:closed, opts)
-    |> update_conn_state(:closed, opts)
-  end
-
   defp generate_ssrcs(state) do
     generate_ssrcs(state.transceivers, Map.keys(state.demuxer.ssrc_to_mid))
   end

test/ex_webrtc/peer_connection_test.exs

@@ -1129,32 +1129,30 @@ defmodule ExWebRTC.PeerConnectionTest do
 
       assert :ok = PeerConnection.close(pc2)
 
-      assert_receive {:ex_webrtc, ^pc1, {:signaling_state_change, :closed}}
-      assert_receive {:ex_webrtc, ^pc1, {:ice_connection_state_change, :closed}}
+      # only dtls transport should move to the closed state
+      # this way peer connection can do ice restart with a different peer
       assert_receive {:ex_webrtc, ^pc1, {:dtls_transport_state_change, :closed}}
-      assert_receive {:ex_webrtc, ^pc1, {:connection_state_change, :closed}}
+      refute_receive {:ex_webrtc, ^pc1, {:signaling_state_change, :closed}}
+      refute_receive {:ex_webrtc, ^pc1, {:ice_connection_state_change, :closed}}
+      refute_receive {:ex_webrtc, ^pc1, {:connection_state_change, :closed}}
 
       # the process should still be alive
       assert true = Process.alive?(pc1)
 
       # Here, we don't really know what should be the state of transceivers and data channels.
       # W3C and browser behavior are unclear at the moment of writing this tes.
       # To be consistent, we just mark them as stopped/closed.
-      [%{stopped: true}] = PeerConnection.get_transceivers(pc1)
-      %{ready_state: :closed} = PeerConnection.get_data_channel(pc1, data_channel.ref)
+      [%{stopped: false}] = PeerConnection.get_transceivers(pc1)
+      %{ready_state: :open} = PeerConnection.get_data_channel(pc1, data_channel.ref)
 
-      # Try to send RTP, it should be ignored
-      packet = ExRTP.Packet.new(<<1, 2, 3>>)
-      :ok = PeerConnection.send_rtp(pc1, track.id, packet)
-      refute_receive {:ex_webrtc, ^pc2, {:rtp, _track_id, nil, _packet}}
+      # try to do ice-restart with a different peer
+      {:ok, pc3} = PeerConnection.start_link()
 
-      # Try to send data, it should be ignored
-      :ok = PeerConnection.send_data(pc1, data_channel.ref, <<1, 2, 3>>)
-      refute_receive {:ex_webrtc, ^pc2, {:data, _dc_ref, _data}}
+      assert :ok = negotiate(pc1, pc3, ice_restart: true)
+      assert :ok = connect(pc1, pc3)
 
-      # Try to send RTCP, it should be ignored
-      :ok = PeerConnection.send_pli(pc1, track.id)
-      refute_receive {:ex_webrtc, ^pc2, {:rtcp, _}}
+      :ok = PeerConnection.send_rtp(pc1, track.id, ExRTP.Packet.new(<<1, 2, 3>>))
+      assert_receive {:ex_webrtc, ^pc3, {:rtp, _track_id, nil, _packet}}
     end
   end
 

test/support/test_utils.ex

@@ -3,9 +3,10 @@ defmodule ExWebRTC.Support.TestUtils do
 
   alias ExWebRTC.PeerConnection
 
-  @spec negotiate(PeerConnection.peer_connection(), PeerConnection.peer_connection()) :: :ok
-  def negotiate(pc1, pc2) do
-    {:ok, offer} = PeerConnection.create_offer(pc1)
+  @spec negotiate(PeerConnection.peer_connection(), PeerConnection.peer_connection(), Keyword.t()) ::
+          :ok
+  def negotiate(pc1, pc2, opts \\ []) do
+    {:ok, offer} = PeerConnection.create_offer(pc1, ice_restart: opts[:ice_restart] || false)
     :ok = PeerConnection.set_local_description(pc1, offer)
     :ok = PeerConnection.set_remote_description(pc2, offer)
     {:ok, answer} = PeerConnection.create_answer(pc2)