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 =
@@ -241,9 +261,15 @@ 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
+        # cleare remote info in case we are re-initializing dtls transport
+        remote_cert: nil,
+        remote_base64_cert: nil,
+        remote_fingerprint: nil
     }
 
     {:reply, :ok, state}
@@ -262,9 +288,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
 
@@ -360,8 +384,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 +405,20 @@ 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.
+    # IMPORTNAT!!!
+    # 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 still sends 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 +576,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,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 +2560,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

mix.exs

@@ -62,7 +62,7 @@ defmodule ExWebRTC.MixProject do
       {:ex_ice, github: "elixir-webrtc/ex_ice", branch: "close"},
       # {:ex_dtls, "~> 0.17.0"},
       # {:ex_dtls, path: "/home/michal/Repos/elixir-webrtc/ex_dtls"},
-      {:ex_dtls, github: "elixir-webrtc/ex_dtls", branch: "disconnect"},
+      {:ex_dtls, github: "elixir-webrtc/ex_dtls"},
       {:ex_libsrtp, "~> 0.7.1"},
       {:ex_rtp, "~> 0.4.0"},
       {:ex_rtcp, "~> 0.4.0"},

mix.lock

@@ -11,7 +11,7 @@
   "elixir_uuid": {:hex, :elixir_uuid, "1.2.1", "dce506597acb7e6b0daeaff52ff6a9043f5919a4c3315abb4143f0b00378c097", [:mix], [], "hexpm", "f7eba2ea6c3555cea09706492716b0d87397b88946e6380898c2889d68585752"},
   "erlex": {:hex, :erlex, "0.2.7", "810e8725f96ab74d17aac676e748627a07bc87eb950d2b83acd29dc047a30595", [:mix], [], "hexpm", "3ed95f79d1a844c3f6bf0cea61e0d5612a42ce56da9c03f01df538685365efb0"},
   "ex_doc": {:hex, :ex_doc, "0.38.2", "504d25eef296b4dec3b8e33e810bc8b5344d565998cd83914ffe1b8503737c02", [:mix], [{:earmark_parser, "~> 1.4.44", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_c, ">= 0.1.0", [hex: :makeup_c, repo: "hexpm", optional: true]}, {:makeup_elixir, "~> 0.14 or ~> 1.0", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1 or ~> 1.0", [hex: :makeup_erlang, repo: "hexpm", optional: false]}, {:makeup_html, ">= 0.1.0", [hex: :makeup_html, repo: "hexpm", optional: true]}], "hexpm", "732f2d972e42c116a70802f9898c51b54916e542cc50968ac6980512ec90f42b"},
-  "ex_dtls": {:git, "https://github.com/elixir-webrtc/ex_dtls.git", "fb6d85d7e285154a2ad68d69ccd30d423efe6f55", [branch: "disconnect"]},
+  "ex_dtls": {:git, "https://github.com/elixir-webrtc/ex_dtls.git", "2773c01f0593cf31d5bf5534c40b2def17abc921", []},
   "ex_ice": {:git, "https://github.com/elixir-webrtc/ex_ice.git", "c13e5ad9d379efc5cad3977b3676cf5c34113a73", [branch: "close"]},
   "ex_libsrtp": {:hex, :ex_libsrtp, "0.7.2", "211bd89c08026943ce71f3e2c0231795b99cee748808ed3ae7b97cd8d2450b6b", [:mix], [{:bunch, "~> 1.6", [hex: :bunch, repo: "hexpm", optional: false]}, {:bundlex, "~> 1.3", [hex: :bundlex, repo: "hexpm", optional: false]}, {:membrane_precompiled_dependency_provider, "~> 0.1.0", [hex: :membrane_precompiled_dependency_provider, repo: "hexpm", optional: false]}, {:unifex, "~> 1.1", [hex: :unifex, repo: "hexpm", optional: false]}], "hexpm", "2e20645d0d739a4ecdcf8d4810a0c198120c8a2f617f2b75b2e2e704d59f492a"},
   "ex_rtcp": {:hex, :ex_rtcp, "0.4.0", "f9e515462a9581798ff6413583a25174cfd2101c94a2ebee871cca7639886f0a", [:mix], [], "hexpm", "28956602cf210d692fcdaf3f60ca49681634e1deb28ace41246aee61ee22dc3b"},