agent: Add assertions to check component state transitions are valid

There is no point in the NiceComponents having a state machine if the
state transition graph is not documented or enforced. Document and
enforce it.

http://phabricator.freedesktop.org/T120

5 files changed, 119 insertions, 19 deletions

diff --git a/agent/agent.c b/agent/agent.c
index 09945f1..6b20ddf 100644
--- a/agent/agent.c
+++ b/agent/agent.c
@@ -713,9 +713,12 @@ nice_agent_class_init (NiceAgentClass *klass)
    * @agent: The #NiceAgent object
    * @stream_id: The ID of the stream
    * @component_id: The ID of the component
-   * @state: The #NiceComponentState of the component
+   * @state: The new #NiceComponentState of the component
    *
-   * This signal is fired whenever a component's state changes
+   * This signal is fired whenever a component’s state changes. There are many
+   * valid state transitions.
+   *
+   * ![State transition diagram](states.png)
    */
   signals[SIGNAL_COMPONENT_STATE_CHANGED] =
       g_signal_new (
@@ -2121,28 +2124,65 @@ nice_component_state_to_string (NiceComponentState state)
     }
 }
 
-void agent_signal_component_state_change (NiceAgent *agent, guint stream_id, guint component_id, NiceComponentState state)
+void agent_signal_component_state_change (NiceAgent *agent, guint stream_id, guint component_id, NiceComponentState new_state)
 {
+  NiceComponentState old_state;
   Component *component;
   Stream *stream;
 
+  g_return_if_fail (new_state < NICE_COMPONENT_STATE_LAST);
+
   if (!agent_find_component (agent, stream_id, component_id,
           &stream, &component))
     return;
 
-  if (component->state != state && state < NICE_COMPONENT_STATE_LAST) {
-    nice_debug ("Agent %p : stream %u component %u STATE-CHANGE %s -> %s.", agent,
-        stream_id, component_id, nice_component_state_to_string (component->state),
-        nice_component_state_to_string (state));
+  /* Validate the state change. */
+  old_state = component->state;
 
-    component->state = state;
+  if (new_state == old_state) {
+    return;
+  }
 
-    if (agent->reliable)
-      process_queued_tcp_packets (agent, stream, component);
+  nice_debug ("Agent %p : stream %u component %u STATE-CHANGE %s -> %s.", agent,
+      stream_id, component_id, nice_component_state_to_string (old_state),
+      nice_component_state_to_string (new_state));
+
+  /* Check whether it’s a valid state transition. */
+#define TRANSITION(OLD, NEW) \
+  (old_state == NICE_COMPONENT_STATE_##OLD && \
+   new_state == NICE_COMPONENT_STATE_##NEW)
+
+  g_assert (/* Can (almost) always transition to FAILED (including
+             * DISCONNECTED → FAILED which happens if one component fails
+             * before another leaves DISCONNECTED): */
+            TRANSITION (DISCONNECTED, FAILED) ||
+            TRANSITION (GATHERING, FAILED) ||
+            TRANSITION (CONNECTING, FAILED) ||
+            TRANSITION (CONNECTED, FAILED) ||
+            TRANSITION (READY, FAILED) ||
+            /* Standard progression towards a ready connection: */
+            TRANSITION (DISCONNECTED, GATHERING) ||
+            TRANSITION (GATHERING, CONNECTING) ||
+            TRANSITION (CONNECTING, CONNECTED) ||
+            TRANSITION (CONNECTED, READY) ||
+            /* priv_conn_check_add_for_candidate_pair_matched(): */
+            TRANSITION (READY, CONNECTED) ||
+            /* If set_remote_candidates() is called with new candidates after
+             * reaching FAILED: */
+            TRANSITION (FAILED, CONNECTING) ||
+            /* Possible by calling set_remote_candidates() without calling
+             * nice_agent_gather_candidates(): */
+            TRANSITION (DISCONNECTED, CONNECTING));
+
+#undef TRANSITION
+
+  component->state = new_state;
 
-    agent_queue_signal (agent, signals[SIGNAL_COMPONENT_STATE_CHANGED],
-        stream_id, component_id, state);
-  }
+  if (agent->reliable)
+    process_queued_tcp_packets (agent, stream, component);
+
+  agent_queue_signal (agent, signals[SIGNAL_COMPONENT_STATE_CHANGED],
+      stream_id, component_id, new_state);
 }
 
 guint64
@@ -5044,8 +5084,18 @@ nice_agent_set_selected_pair (
     goto done;
   }
 
-  /* step: change component state */
-  agent_signal_component_state_change (agent, stream_id, component_id, NICE_COMPONENT_STATE_READY);
+  /* step: change component state; we could be in STATE_DISCONNECTED; skip
+   * STATE_GATHERING and continue through the states to give client code a nice
+   * logical progression. See http://phabricator.freedesktop.org/D218 for
+   * discussion. */
+  if (component->state < NICE_COMPONENT_STATE_CONNECTING)
+    agent_signal_component_state_change (agent, stream_id, component_id,
+        NICE_COMPONENT_STATE_CONNECTING);
+  if (component->state < NICE_COMPONENT_STATE_CONNECTED)
+    agent_signal_component_state_change (agent, stream_id, component_id,
+        NICE_COMPONENT_STATE_CONNECTED);
+  agent_signal_component_state_change (agent, stream_id, component_id,
+      NICE_COMPONENT_STATE_READY);
 
   /* step: set the selected pair */
   component_update_selected_pair (component, &pair);
@@ -5222,8 +5272,18 @@ nice_agent_set_selected_remote_candidate (
     goto done;
   }
 
-  /* step: change component state */
-  agent_signal_component_state_change (agent, stream_id, component_id, NICE_COMPONENT_STATE_READY);
+  /* step: change component state; we could be in STATE_DISCONNECTED; skip
+   * STATE_GATHERING and continue through the states to give client code a nice
+   * logical progression. See http://phabricator.freedesktop.org/D218 for
+   * discussion. */
+  if (component->state < NICE_COMPONENT_STATE_CONNECTING)
+    agent_signal_component_state_change (agent, stream_id, component_id,
+        NICE_COMPONENT_STATE_CONNECTING);
+  if (component->state < NICE_COMPONENT_STATE_CONNECTED)
+    agent_signal_component_state_change (agent, stream_id, component_id,
+        NICE_COMPONENT_STATE_CONNECTED);
+  agent_signal_component_state_change (agent, stream_id, component_id,
+      NICE_COMPONENT_STATE_READY);
 
   agent_signal_new_selected_pair (agent, stream_id, component_id,
       lcandidate, candidate);
diff --git a/agent/conncheck.c b/agent/conncheck.c
index 1b687b6..7832339 100644
--- a/agent/conncheck.c
+++ b/agent/conncheck.c
@@ -1326,6 +1326,15 @@ static void priv_update_check_list_state_for_ready (NiceAgent *agent, Stream *st
      * any that are kept, then this function will be called again when the
      * conncheck tick timer finishes them all */
     if (priv_prune_pending_checks (stream, component->id) == 0) {
+      /* Continue through the states to give client code a nice
+       * logical progression. See http://phabricator.freedesktop.org/D218 for
+       * discussion. */
+      if (component->state < NICE_COMPONENT_STATE_CONNECTING)
+        agent_signal_component_state_change (agent, stream->id, component->id,
+            NICE_COMPONENT_STATE_CONNECTING);
+      if (component->state < NICE_COMPONENT_STATE_CONNECTED)
+        agent_signal_component_state_change (agent, stream->id, component->id,
+            NICE_COMPONENT_STATE_CONNECTED);
       agent_signal_component_state_change (agent, stream->id,
           component->id, NICE_COMPONENT_STATE_READY);
     }
diff --git a/docs/reference/libnice/Makefile.am b/docs/reference/libnice/Makefile.am
index 1d53e3b..19e479e 100644
--- a/docs/reference/libnice/Makefile.am
+++ b/docs/reference/libnice/Makefile.am
@@ -62,7 +62,7 @@ IGNORE_HFILES= conncheck.h discovery.h stream.h component.h agent-priv.h \
 
 # Images to copy into HTML directory.
 # e.g. HTML_IMAGES=$(top_srcdir)/gtk/stock-icons/stock_about_24.png
-HTML_IMAGES=
+HTML_IMAGES = states.png
 
 # Extra SGML files that are included by $(DOC_MAIN_SGML_FILE).
 # e.g. content_files=running.sgml building.sgml changes-2.0.sgml
@@ -94,13 +94,19 @@ include $(top_srcdir)/gtk-doc.make
 
 # Other files to distribute
 # e.g. EXTRA_DIST += version.xml.in
-#EXTRA_DIST +=
+EXTRA_DIST += states.gv
 
 # Files not to distribute
 # for --rebuild-types in $(SCAN_OPTIONS), e.g. $(DOC_MODULE).types
 # for --rebuild-sections in $(SCAN_OPTIONS) e.g. $(DOC_MODULE)-sections.txt
 #DISTCLEANFILES +=
 
+# If we ever need to regenerate this diagram.
+# Since it’s not expected to change much, let’s not depend on GraphViz to
+# build the docs.
+states.png: states.gv
+	dot -Tpng -Gsize=9.6,2.9\! -Gdpi=200 $^ > $@
+
 if ENABLE_GTK_DOC
 TESTS_ENVIRONMENT = cd $(builddir) &&
 TESTS = $(GTKDOC_CHECK)
diff --git a/docs/reference/libnice/states.gv b/docs/reference/libnice/states.gv
new file mode 100644
index 0000000..609be2e
--- /dev/null
+++ b/docs/reference/libnice/states.gv
@@ -0,0 +1,25 @@
+/* libnice state transition diagram for NiceComponentState. */
+digraph NiceComponentState {
+	rankdir=TB;
+	node [shape = doublecircle]; DISCONNECTED;
+	node [shape = circle];
+
+	/* Colour the normal control flow in green. */
+	DISCONNECTED -> GATHERING [ label = "nice_agent_gather_candidates()", color = chartreuse3 ];
+	GATHERING -> CONNECTING [ label = "nice_agent_set_remote_candidates()", color = chartreuse3 ];
+	CONNECTING -> CONNECTED [ label = "At least one candidate pair succeeds", color = chartreuse3 ];
+	CONNECTED -> READY [ label = "All candidate pairs checks finished", color = chartreuse3 ];
+
+	READY -> CONNECTED [ label = "Selected candidate pair fails" ];
+
+	FAILED -> CONNECTING [ label = "nice_agent_set_remote_candidates()" ];
+
+	DISCONNECTED -> CONNECTING [ label = "nice_agent_set_remote_candidates()" ];
+
+	/* Colour the failure paths in grey. */
+	DISCONNECTED -> FAILED [ label = "Failure", color = gray ];
+	GATHERING -> FAILED [ label = "Failure", color = gray ];
+	CONNECTING -> FAILED [ label = "Failure", color = gray ];
+	CONNECTED -> FAILED [ label = "Failure", color = gray ];
+	READY -> FAILED [ label = "Failure", color = gray ];
+}
diff --git a/docs/reference/libnice/states.png b/docs/reference/libnice/states.png
new file mode 100644
index 0000000..ba23739
--- /dev/null
+++ b/docs/reference/libnice/states.png