Commit 2eaa8b32 authored by Philip Withnall's avatar Philip Withnall Committed by Philip Withnall

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
parent 3f54b333
...@@ -713,9 +713,12 @@ nice_agent_class_init (NiceAgentClass *klass) ...@@ -713,9 +713,12 @@ nice_agent_class_init (NiceAgentClass *klass)
* @agent: The #NiceAgent object * @agent: The #NiceAgent object
* @stream_id: The ID of the stream * @stream_id: The ID of the stream
* @component_id: The ID of the component * @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] = signals[SIGNAL_COMPONENT_STATE_CHANGED] =
g_signal_new ( g_signal_new (
...@@ -2121,28 +2124,65 @@ nice_component_state_to_string (NiceComponentState state) ...@@ -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; Component *component;
Stream *stream; Stream *stream;
g_return_if_fail (new_state < NICE_COMPONENT_STATE_LAST);
if (!agent_find_component (agent, stream_id, component_id, if (!agent_find_component (agent, stream_id, component_id,
&stream, &component)) &stream, &component))
return; return;
if (component->state != state && state < NICE_COMPONENT_STATE_LAST) { /* Validate the state change. */
nice_debug ("Agent %p : stream %u component %u STATE-CHANGE %s -> %s.", agent, old_state = component->state;
stream_id, component_id, nice_component_state_to_string (component->state),
nice_component_state_to_string (state)); if (new_state == old_state) {
return;
}
component->state = state; 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;
if (agent->reliable) if (agent->reliable)
process_queued_tcp_packets (agent, stream, component); process_queued_tcp_packets (agent, stream, component);
agent_queue_signal (agent, signals[SIGNAL_COMPONENT_STATE_CHANGED], agent_queue_signal (agent, signals[SIGNAL_COMPONENT_STATE_CHANGED],
stream_id, component_id, state); stream_id, component_id, new_state);
}
} }
guint64 guint64
...@@ -5044,8 +5084,18 @@ nice_agent_set_selected_pair ( ...@@ -5044,8 +5084,18 @@ nice_agent_set_selected_pair (
goto done; goto done;
} }
/* step: change component state */ /* step: change component state; we could be in STATE_DISCONNECTED; skip
agent_signal_component_state_change (agent, stream_id, component_id, NICE_COMPONENT_STATE_READY); * 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 */ /* step: set the selected pair */
component_update_selected_pair (component, &pair); component_update_selected_pair (component, &pair);
...@@ -5222,8 +5272,18 @@ nice_agent_set_selected_remote_candidate ( ...@@ -5222,8 +5272,18 @@ nice_agent_set_selected_remote_candidate (
goto done; goto done;
} }
/* step: change component state */ /* step: change component state; we could be in STATE_DISCONNECTED; skip
agent_signal_component_state_change (agent, stream_id, component_id, NICE_COMPONENT_STATE_READY); * 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, agent_signal_new_selected_pair (agent, stream_id, component_id,
lcandidate, candidate); lcandidate, candidate);
......
...@@ -1326,6 +1326,15 @@ static void priv_update_check_list_state_for_ready (NiceAgent *agent, Stream *st ...@@ -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 * any that are kept, then this function will be called again when the
* conncheck tick timer finishes them all */ * conncheck tick timer finishes them all */
if (priv_prune_pending_checks (stream, component->id) == 0) { 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, agent_signal_component_state_change (agent, stream->id,
component->id, NICE_COMPONENT_STATE_READY); component->id, NICE_COMPONENT_STATE_READY);
} }
......
...@@ -62,7 +62,7 @@ IGNORE_HFILES= conncheck.h discovery.h stream.h component.h agent-priv.h \ ...@@ -62,7 +62,7 @@ IGNORE_HFILES= conncheck.h discovery.h stream.h component.h agent-priv.h \
# Images to copy into HTML directory. # Images to copy into HTML directory.
# e.g. HTML_IMAGES=$(top_srcdir)/gtk/stock-icons/stock_about_24.png # 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). # Extra SGML files that are included by $(DOC_MAIN_SGML_FILE).
# e.g. content_files=running.sgml building.sgml changes-2.0.sgml # e.g. content_files=running.sgml building.sgml changes-2.0.sgml
...@@ -94,13 +94,19 @@ include $(top_srcdir)/gtk-doc.make ...@@ -94,13 +94,19 @@ include $(top_srcdir)/gtk-doc.make
# Other files to distribute # Other files to distribute
# e.g. EXTRA_DIST += version.xml.in # e.g. EXTRA_DIST += version.xml.in
#EXTRA_DIST += EXTRA_DIST += states.gv
# Files not to distribute # Files not to distribute
# for --rebuild-types in $(SCAN_OPTIONS), e.g. $(DOC_MODULE).types # for --rebuild-types in $(SCAN_OPTIONS), e.g. $(DOC_MODULE).types
# for --rebuild-sections in $(SCAN_OPTIONS) e.g. $(DOC_MODULE)-sections.txt # for --rebuild-sections in $(SCAN_OPTIONS) e.g. $(DOC_MODULE)-sections.txt
#DISTCLEANFILES += #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 if ENABLE_GTK_DOC
TESTS_ENVIRONMENT = cd $(builddir) && TESTS_ENVIRONMENT = cd $(builddir) &&
TESTS = $(GTKDOC_CHECK) TESTS = $(GTKDOC_CHECK)
......
/* 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 ];
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment