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

Author: mickel8

examples/chat/lib/chat/peer_handler.ex

@@ -106,8 +106,8 @@ defmodule Chat.PeerHandler do
   defp handle_webrtc_msg({:connection_state_change, conn_state}, state) do
     Logger.info("Connection state changed: #{conn_state}")
 
-    if conn_state in [:failed, :closed] do
-      {:stop, {:shutdown, :pc_failed_or_closed}, state}
+    if conn_state == :failed do
+      {:stop, {:shutdown, :pc_failed}, state}
     else
       {:ok, state}
     end

examples/dtmf/lib/dtmf/peer_handler.ex

@@ -125,8 +125,8 @@ defmodule Dtmf.PeerHandler do
   defp handle_webrtc_msg({:connection_state_change, conn_state}, state) do
     Logger.info("Connection state changed: #{conn_state}")
 
-    if conn_state in [:failed, :closed] do
-      {:stop, {:shutdown, :pc_failed_or_closed}, state}
+    if conn_state == :failed do
+      {:stop, {:shutdown, :pc_failed}, state}
     else
       {:ok, state}
     end

examples/echo/lib/echo/peer_handler.ex

@@ -115,8 +115,8 @@ defmodule Echo.PeerHandler do
   defp handle_webrtc_msg({:connection_state_change, conn_state}, state) do
     Logger.info("Connection state changed: #{conn_state}")
 
-    if conn_state in [:failed, :closed] do
-      {:stop, {:shutdown, :pc_failed_or_closed}, state}
+    if conn_state == :failed do
+      {:stop, {:shutdown, :pc_failed}, state}
     else
       {:ok, state}
     end

examples/save_to_file/lib/save_to_file/peer_handler.ex

@@ -136,8 +136,8 @@ defmodule SaveToFile.PeerHandler do
   defp handle_webrtc_msg({:connection_state_change, conn_state}, state) do
     Logger.info("Connection state changed: #{conn_state}")
 
-    if conn_state in [:failed, :closed] do
-      {:stop, {:shutdown, :pc_failed_or_closed}, state}
+    if conn_state == :failed do
+      {:stop, {:shutdown, :pc_failed}, state}
     else
       {:ok, state}
     end

examples/send_from_file/lib/send_from_file/peer_handler.ex

@@ -249,8 +249,8 @@ defmodule SendFromFile.PeerHandler do
   defp handle_webrtc_msg({:connection_state_change, conn_state}, state) do
     Logger.info("Connection state changed: #{conn_state}")
 
-    if conn_state in [:failed, :closed] do
-      {:stop, {:shutdown, :pc_failed_or_closed}, state}
+    if conn_state == :failed do
+      {:stop, {:shutdown, :pc_failed}, state}
     else
       {:ok, state}
     end

examples/whip_whep/lib/whip_whep/forwarder.ex

@@ -69,16 +69,12 @@ defmodule WhipWhep.Forwarder do
   end
 
   @impl true
-  def handle_info(
-        {:ex_webrtc, pc, {:connection_state_change, conn_state}},
-        %{input_pc: pc} = state
-      )
-      when conn_state in [:failed, :closed] do
-    Logger.info("Input peer connection (#{inspect(pc)}) state change: #{conn_state}. Removing.")
+  def handle_info({:ex_webrtc, pc, {:connection_state_change, :failed}}, %{input_pc: pc} = state) do
+    Logger.info("Input peer connection (#{inspect(pc)}) state change: failed. Removing.")
     :ok = PeerConnection.stop(state.input_pc)
 
-    Enum.each(state.pending_outputs, &PeerConnection.close(&1))
-    Enum.each(state.outputs, fn {pc, _output} -> PeerConnection.close(pc) end)
+    Enum.each(state.pending_outputs, &PeerConnection.stop(&1))
+    Enum.each(state.outputs, fn {pc, _output} -> PeerConnection.stop(pc) end)
 
     state = %{
       state
@@ -124,9 +120,8 @@ defmodule WhipWhep.Forwarder do
   end
 
   @impl true
-  def handle_info({:ex_webrtc, pc, {:connection_state_change, conn_state}}, state)
-      when conn_state in [:failed, :closed] do
-    Logger.info("Output peer connection (#{inspect(pc)}) state change: #{conn_state}. Removing.")
+  def handle_info({:ex_webrtc, pc, {:connection_state_change, :failed}}, state) do
+    Logger.info("Output peer connection (#{inspect(pc)}) state change: failed. Removing.")
     :ok = PeerConnection.stop(pc)
     pending_outputs = List.delete(state.pending_outputs, pc)
     outputs = Map.delete(state.outputs, pc)

lib/ex_webrtc/dtls_transport.ex

@@ -62,6 +62,11 @@ defmodule ExWebRTC.DTLSTransport do
     GenServer.call(dtls_transport, :set_ice_connected)
   end
 
+  @spec set_ice_disconnected(dtls_transport()) :: :ok
+  def set_ice_disconnected(dtls_transport) do
+    GenServer.call(dtls_transport, :set_ice_disconnected)
+  end
+
   @spec get_certs_info(dtls_transport()) :: %{
           local_cert_info: cert_info(),
           remote_cert_info: cert_info() | nil
@@ -131,8 +136,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,
@@ -187,6 +192,12 @@ defmodule ExWebRTC.DTLSTransport do
     {:reply, :ok, state}
   end
 
+  @impl true
+  def handle_call(:set_ice_disconnected, _from, state) do
+    state = %{state | ice_connected: false}
+    {:reply, :ok, state}
+  end
+
   @impl true
   def handle_call(:get_certs_info, _from, state) do
     local_cert_info = %{
@@ -217,9 +228,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 =
@@ -238,13 +258,20 @@ defmodule ExWebRTC.DTLSTransport do
       data -> send(self(), {:ex_ice, state.ice_pid, {:data, data}})
     end
 
-    state = %{
-      state
-      | dtls: dtls,
-        mode: mode,
-        peer_fingerprint: peer_fingerprint,
-        buffered_remote_packets: nil
-    }
+    state =
+      %{
+        state
+        | dtls: dtls,
+          mode: mode,
+          in_srtp: ExLibSRTP.new(),
+          out_srtp: ExLibSRTP.new(),
+          peer_fingerprint: peer_fingerprint,
+          # clear remote info in case we are re-initializing dtls transport
+          remote_cert: nil,
+          remote_base64_cert: nil,
+          remote_fingerprint: nil
+      }
+      |> update_dtls_state(:new)
 
     {:reply, :ok, state}
   end
@@ -262,9 +289,7 @@ defmodule ExWebRTC.DTLSTransport do
 
   @impl true
   def handle_call(:close, _from, state) do
-    {:ok, packets} = ExDTLS.close(state.dtls)
-    :ok = do_send(state, packets)
-    state = update_dtls_state(state, :closed, notify: false)
+    state = do_close(state, notify: false)
     {:reply, :ok, state}
   end
 
@@ -322,6 +347,13 @@ defmodule ExWebRTC.DTLSTransport do
     {:noreply, state}
   end
 
+  @impl true
+  def handle_info(:dtls_timeout, %{dtls: nil} = state) do
+    # ignore stale timers that were scheduled before calling
+    # close/1 or receiving close_notify DTLS alert
+    {:noreply, state}
+  end
+
   @impl true
   def handle_info(:dtls_timeout, %{buffered_local_packets: buffered_local_packets} = state) do
     case ExDTLS.handle_timeout(state.dtls) do
@@ -360,8 +392,7 @@ defmodule ExWebRTC.DTLSTransport do
         # See W3C WebRTC sec. 5.5.1
         # peer_closed_for_writing is returned when the remote side
         # sends close_notify alert
-        ExDTLS.close(state.dtls)
-        state = update_dtls_state(state, :closed)
+        state = do_close(state)
         {:noreply, state}
 
       {:error, _reason} ->
@@ -382,9 +413,19 @@ defmodule ExWebRTC.DTLSTransport do
     Logger.debug("Stopping DTLSTransport with reason: #{inspect(reason)}")
   end
 
-  defp handle_ice_data({:data, _data}, %{dtls_state: dtls_state} = state)
-       when dtls_state in [:failed, :closed] do
-    Logger.debug("Received DTLS packets in state #{dtls_state}. Ignoring.")
+  defp handle_ice_data({:data, _data}, %{dtls_state: :closed} = state) do
+    # Ignore incoming packets when we are in the closed state.
+    # IMPORTANT!!! This isn't optimal.
+    # When DTLSTransport has been closed because of receiving close_notify alert,
+    # it can still be re-used after doing an ice restart with a different peer.
+    # In such case, we might start getting remote ICE packets before getting remote SDP answer
+    # (see the case clause below).
+    # These packets could be buffered and processed once we receive said SDP answer to speedup
+    # connection establishment time (instead of waiting for retransmissions).
+    # However, it is hard to differentiate this case from a case where DTLSTransport received
+    # close_notify alert, but the remote side behaves incorrectly and is still sending ICE packets.
+    # Buffering incoming packets in closed state, we don't know whether they belong to current or previous
+    # session, which can lead to hard to find bugs.
     {:ok, state}
   end
 
@@ -542,6 +583,23 @@ defmodule ExWebRTC.DTLSTransport do
     %{state | buffered_remote_rtp_packets: []}
   end
 
+  defp do_close(state, opts \\ []) do
+    {:ok, packets} = ExDTLS.close(state.dtls)
+    :ok = do_send(state, packets)
+
+    %{
+      state
+      | buffered_local_packets: nil,
+        buffered_remote_packets: nil,
+        buffered_remote_rtp_packets: [],
+        in_srtp: nil,
+        out_srtp: nil,
+        dtls: nil,
+        mode: nil
+    }
+    |> update_dtls_state(:closed, opts)
+  end
+
   defp do_send(state, data) when is_list(data) do
     Enum.each(data, &(:ok = do_send(state, &1)))
   end

lib/ex_webrtc/peer_connection.ex

@@ -685,7 +685,7 @@ defmodule ExWebRTC.PeerConnection do
 
   @impl true
   def handle_call(
-        {:set_sender_code, _sender_id, _codec},
+        {:set_sender_codec, _sender_id, _codec},
         _from,
         %{signaling_state: :closed} = state
       ) do
@@ -1151,7 +1151,7 @@ defmodule ExWebRTC.PeerConnection do
     def handle_call(
           {:create_data_channel, _label, _opts},
           _from,
-          %{connection_state: :closed} = state
+          %{signaling_state: :closed} = state
         ) do
       {:reply, {:error, :invalid_state}, state}
     end
@@ -1382,14 +1382,28 @@ defmodule ExWebRTC.PeerConnection do
 
   @impl true
   def handle_call(:close, _from, %{signaling_state: :closed} = state) do
-    {:reply, {:error, :invalid_state}, state}
+    {:reply, :ok, state}
   end
 
   @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
 
@@ -1518,6 +1532,10 @@ defmodule ExWebRTC.PeerConnection do
       :ok = DTLSTransport.set_ice_connected(state.dtls_transport)
     end
 
+    if new_ice_state == :failed do
+      :ok = DTLSTransport.set_ice_disconnected(state.dtls_transport)
+    end
+
     {:noreply, state}
   end
 
@@ -1548,17 +1566,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
@@ -2325,13 +2338,8 @@ defmodule ExWebRTC.PeerConnection do
   # the order of these clauses is important
   defp next_conn_state(ice_state, dtls_state)
 
-  # Give closed precedence over failed.
-  # Failed connection can be restarted.
-  # 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 +2350,13 @@ 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
+  # DTLS closed state does not indicate peer connection closed state.
+  # When DTLS was closed by receiving close_notify alert, it can still
+  # be reused after doing an ice-restart with a different peer.
+  # See https://github.com/w3c/webrtc-pc/issues/3053.
+  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 +2564,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