conncheck: remove cancelled pair state

Pairs with the state NICE_CHECK_CANCELLED are the pairs targeted for
removal after the nomination of a pair with an higher priority,
described in Section 8.1.2 "Updating States", item 2 of RFC 5245. They
include also pairs that overflow the conncheck list size, but this is a
somewhat more marginal situation. So we are mainly interested in the
first use case of this state.

This state mixes two different situations, that deserve a distinct
handling : on one side, there are waiting or frozen pairs that must be
removed, this is an immediate action that doesn't need a dedicated state
for that. And on the other side, there are in-progress pairs that
should no longer be retransmitted, because another pair with a higher
priority has already been nominated.

This patch removes the cancelled state, and adds a flag
retransmit_on_timeout to deal with this last situation. Note that this
case should not generate a triggered check, as per described in section
7.2.1.4, when the state of the pair is In-Progress or Failed, since this
pair of lower priority has no hope to replace the nominated one.

Differential Revision: https://phabricator.freedesktop.org/D1114

2 files changed, 77 insertions, 68 deletions

diff --git a/agent/conncheck.c b/agent/conncheck.c
index 88d2534..b0e2222 100644
--- a/agent/conncheck.c
+++ b/agent/conncheck.c
@@ -100,8 +100,6 @@ priv_state_to_gchar (NiceCheckState state)
       return 'F';
     case NICE_CHECK_FROZEN:
       return 'Z';
-    case NICE_CHECK_CANCELLED:
-      return 'C';
     case NICE_CHECK_DISCOVERED:
       return 'D';
     default:
@@ -627,6 +625,7 @@ static gboolean priv_conn_check_tick_stream (NiceStream *stream, NiceAgent *agen
               gchar tmpbuf1[INET6_ADDRSTRLEN], tmpbuf2[INET6_ADDRSTRLEN];
               NiceComponent *component;
 
+timer_timeout:
               /* case: conncheck cancelled due to in-progress incoming
                * check, requeing the pair, ICE spec, sect 7.2.1.4
                * "Triggered Checks", "If the state of that pair is
@@ -662,6 +661,13 @@ static gboolean priv_conn_check_tick_stream (NiceStream *stream, NiceAgent *agen
             {
               unsigned int timeout = stun_timer_remainder (&p->timer);
 
+              /* case: retransmission stopped, due to the nomination of
+               * a pair with a higher priority than this in-progress pair,
+               * ICE spec, sect 8.1.2 "Updating States", item 2.2
+               */
+              if (!p->retransmit_on_timeout)
+                goto timer_timeout;
+
               /* case: conncheck cancelled due to in-progress incoming
                * check, requeing the pair, ICE spec, sect 7.2.1.4
                * "Triggered Checks", "If the state of that pair is
@@ -1600,26 +1606,6 @@ static void priv_preprocess_conn_check_pending_data (NiceAgent *agent, NiceStrea
 }
 
 
-static GSList *prune_cancelled_conn_check (GSList *conncheck_list)
-{
-  GSList *item = conncheck_list;
-
-  while (item) {
-    CandidateCheckPair *pair = item->data;
-    GSList *next = item->next;
-
-    if (pair->state == NICE_CHECK_CANCELLED) {
-      conn_check_free_item (pair);
-      conncheck_list = g_slist_delete_link (conncheck_list, item);
-    }
-
-    item = next;
-  }
-
-  return conncheck_list;
-}
-
-
 /*
  * Handle any processing steps for connectivity checks after
  * remote credentials have been set. This function handles
@@ -1754,9 +1740,6 @@ void conn_check_remote_credentials_set(NiceAgent *agent, NiceStream *stream)
         (GDestroyNotify) incoming_check_free);
     component->incoming_checks = NULL;
   }
-
-  stream->conncheck_list =
-      prune_cancelled_conn_check (stream->conncheck_list);
 }
 
 /*
@@ -1764,7 +1747,7 @@ void conn_check_remote_credentials_set(NiceAgent *agent, NiceStream *stream)
  * in ICE spec section 5.7.3 (ID-19). See also 
  * conn_check_add_for_candidate().
  */
-static void priv_limit_conn_check_list_size (GSList *conncheck_list, guint upper_limit)
+static GSList *priv_limit_conn_check_list_size (GSList *conncheck_list, guint upper_limit)
 {
   guint valid = 0;
   guint cancelled = 0;
@@ -1772,22 +1755,22 @@ static void priv_limit_conn_check_list_size (GSList *conncheck_list, guint upper
 
   while (item) {
     CandidateCheckPair *pair = item->data;
+    GSList *next = item->next;
 
-    if (pair->state != NICE_CHECK_CANCELLED) {
-      valid++;
-      if (valid > upper_limit) {
-        pair->state = NICE_CHECK_CANCELLED;
+    valid++;
+    if (valid > upper_limit) {
+        conn_check_free_item (pair);
+        conncheck_list = g_slist_delete_link (conncheck_list, item);
         cancelled++;
-      }
     }
-
-    item = item->next;
+    item = next;
   }
 
   if (cancelled > 0)
     nice_debug ("Agent : Pruned %d candidates. Conncheck list has %d elements"
         " left. Maximum connchecks allowed : %d", cancelled, valid,
         upper_limit);
+  return conncheck_list;
 }
 
 /*
@@ -2097,6 +2080,7 @@ static CandidateCheckPair *priv_add_new_check_pair (NiceAgent *agent,
   }
   pair->prflx_priority = ensure_unique_priority (component,
       peer_reflexive_candidate_priority (agent, local));
+  pair->retransmit_on_timeout = TRUE;
 
   stream->conncheck_list = g_slist_insert_sorted (stream->conncheck_list, pair,
       (GCompareFunc)conn_check_compare);
@@ -2106,7 +2090,8 @@ static CandidateCheckPair *priv_add_new_check_pair (NiceAgent *agent,
   /* implement the hard upper limit for number of
      checks (see sect 5.7.3 ICE ID-19): */
   if (agent->compatibility == NICE_COMPATIBILITY_RFC5245) {
-    priv_limit_conn_check_list_size (stream->conncheck_list, agent->max_conn_checks);
+    stream->conncheck_list = priv_limit_conn_check_list_size (
+        stream->conncheck_list, agent->max_conn_checks);
   }
 
   return pair;
@@ -2769,8 +2754,10 @@ static guint priv_prune_pending_checks (NiceAgent *agent, NiceStream *stream, gu
    * the triggered check list should also be treated like an in-progress
    * pair.
    */
-  for (i = stream->conncheck_list; i; i = i->next) {
+  i = stream->conncheck_list;
+  while (i) {
     CandidateCheckPair *p = i->data;
+    GSList *next = i->next;
 
     if (p->component_id == component_id) {
       gboolean like_in_progress =
@@ -2779,19 +2766,20 @@ static guint priv_prune_pending_checks (NiceAgent *agent, NiceStream *stream, gu
 
       if (p->state == NICE_CHECK_FROZEN ||
           (p->state == NICE_CHECK_WAITING && !like_in_progress)) {
-	p->state = NICE_CHECK_CANCELLED;
-        nice_debug ("Agent %p : pair %p state CANCELED", agent, p);
+        nice_debug ("Agent %p : pair %p removed.", agent, p);
+	conn_check_free_item (p);
+        stream->conncheck_list = g_slist_delete_link(stream->conncheck_list, i);
       }
 
       /* note: a SHOULD level req. in ICE 8.1.2. "Updating States" (ID-19) */
-      if (p->state == NICE_CHECK_IN_PROGRESS ||
+      else if (p->state == NICE_CHECK_IN_PROGRESS ||
           (p->state == NICE_CHECK_WAITING && like_in_progress)) {
         if (highest_nominated_priority != 0 &&
             p->priority < highest_nominated_priority) {
-          p->stun_message.buffer = NULL;
-          p->stun_message.buffer_len = 0;
-          p->state = NICE_CHECK_CANCELLED;
-          nice_debug ("Agent %p : pair %p state CANCELED", agent, p);
+          p->retransmit_on_timeout = FALSE;
+          p->recheck_on_timeout = FALSE;
+          nice_debug ("Agent %p : pair %p will not be retransmitted.",
+              agent, p);
         } else {
           /* We must keep the higher priority pairs running because if a udp
            * packet was lost, we might end up using a bad candidate */
@@ -2803,6 +2791,7 @@ static guint priv_prune_pending_checks (NiceAgent *agent, NiceStream *stream, gu
         }
       }
     }
+    i = next;
   }
 
   return in_progress;
@@ -2841,29 +2830,42 @@ static gboolean priv_schedule_triggered_check (NiceAgent *agent, NiceStream *str
           p = p->succeeded_pair;
         }
 
-	nice_debug ("Agent %p : Found a matching pair %p for triggered check.", agent, p);
+	nice_debug ("Agent %p : Found a matching pair %p (%s) (state=%c) ...",
+            agent, p, p->foundation, priv_state_to_gchar (p->state));
 	
 	if (p->state == NICE_CHECK_WAITING ||
-	    p->state == NICE_CHECK_FROZEN)
+	    p->state == NICE_CHECK_FROZEN) {
+	  nice_debug ("Agent %p : pair %p added for a triggered check.",
+              agent, p);
           priv_add_pair_to_triggered_check_queue (agent, p);
+        }
         else if (p->state == NICE_CHECK_IN_PROGRESS) {
 	  /* note: according to ICE SPEC sect 7.2.1.4 "Triggered Checks"
            * we cancel the in-progress transaction, and after the
            * retransmission timeout, we create a new connectivity check
            * for that pair.  The controlling role of this new check may
            * be different from the role of this cancelled check.
+           *
+           * note: the flag retransmit_on_timeout unset means that
+           * another pair, with a higher priority is already nominated,
+           * so there's no reason to recheck this pair, since it can in
+           * no way replace the nominated one.
            */
           if (!nice_socket_is_reliable (p->sockptr)) {
-            nice_debug ("Agent %p : check already in progress, "
-                "cancelling this check..", agent);
-            /* this flag will determine the action at the retransmission
-             * timeout of the stun timer
-             */
-            p->recheck_on_timeout = TRUE;
+            if (p->retransmit_on_timeout) {
+              nice_debug ("Agent %p : pair %p will be rechecked "
+                  "on stun timer timeout.", agent, p);
+              /* this flag will determine the action at the retransmission
+               * timeout of the stun timer
+               */
+              p->recheck_on_timeout = TRUE;
+            } else
+              nice_debug ("Agent %p : pair %p won't be retransmitted.",
+                  agent, p);
           }
 	}
 	else if (p->state == NICE_CHECK_SUCCEEDED) {
-	  nice_debug ("Agent %p : Skipping triggered check, already completed..", agent); 
+	  nice_debug ("Agent %p : nothing to do for pair %p.", agent, p);
 	  /* note: this is a bit unsure corner-case -- let's do the
 	     same state update as for processing responses to our own checks */
           /* note: this update is required by the dribble test, to
@@ -2875,18 +2877,30 @@ static gboolean priv_schedule_triggered_check (NiceAgent *agent, NiceStream *str
 	} else if (p->state == NICE_CHECK_FAILED) {
           /* 7.2.1.4 Triggered Checks
            * If the state of the pair is Failed, it is changed to Waiting
-             and the agent MUST create a new connectivity check for that
-             pair (representing a new STUN Binding request transaction), by
-             enqueueing the pair in the triggered check queue. */
-          priv_add_pair_to_triggered_check_queue (agent, p);
-          /* If the component for this pair is in failed state, move it
-           * back to connecting, and reinitiate the timers
+           * and the agent MUST create a new connectivity check for that
+           * pair (representing a new STUN Binding request transaction), by
+           * enqueueing the pair in the triggered check queue.
+           *
+           * note: the flag retransmit_on_timeout unset means that
+           * another pair, with a higher priority is already nominated,
+           * we apply the same strategy than with an in-progress pair
+           * above.
            */
-          if (component->state == NICE_COMPONENT_STATE_FAILED) {
-            agent_signal_component_state_change (agent, stream->id,
-                component->id, NICE_COMPONENT_STATE_CONNECTING);
-            conn_check_schedule_next (agent);
-          }
+          if (p->retransmit_on_timeout) {
+	    nice_debug ("Agent %p : pair %p added for a triggered check.",
+                agent, p);
+            priv_add_pair_to_triggered_check_queue (agent, p);
+            /* If the component for this pair is in failed state, move it
+             * back to connecting, and reinitiate the timers
+             */
+            if (component->state == NICE_COMPONENT_STATE_FAILED) {
+              agent_signal_component_state_change (agent, stream->id,
+                  component->id, NICE_COMPONENT_STATE_CONNECTING);
+              conn_check_schedule_next (agent);
+            }
+          } else
+            nice_debug ("Agent %p : pair %p won't be retransmitted.",
+                agent, p);
         }
 
 	/* note: the spec says the we SHOULD retransmit in-progress
@@ -3401,10 +3415,6 @@ static gboolean priv_map_reply_to_conn_check_request (NiceAgent *agent, NiceStre
     }
   }
 
-
-  stream->conncheck_list =
-      prune_cancelled_conn_check (stream->conncheck_list);
-
   return trans_found;
 }
 
diff --git a/agent/conncheck.h b/agent/conncheck.h
index c07fb22..909d469 100644
--- a/agent/conncheck.h
+++ b/agent/conncheck.h
@@ -56,7 +56,6 @@
  * @NICE_CHECK_SUCCEEDED: Connection successfully checked.
  * @NICE_CHECK_FAILED: No connectivity; retransmissions ceased.
  * @NICE_CHECK_FROZEN: Waiting to be scheduled to %NICE_CHECK_WAITING.
- * @NICE_CHECK_CANCELLED: Check cancelled.
  * @NICE_CHECK_DISCOVERED: A valid candidate pair not on the check list.
  *
  * States for checking a candidate pair.
@@ -68,7 +67,6 @@ typedef enum
   NICE_CHECK_SUCCEEDED,
   NICE_CHECK_FAILED,
   NICE_CHECK_FROZEN,
-  NICE_CHECK_CANCELLED,
   NICE_CHECK_DISCOVERED,
 } NiceCheckState;
 
@@ -89,6 +87,7 @@ struct _CandidateCheckPair
   gboolean use_candidate_on_next_check;
   gboolean mark_nominated_on_response_arrival;
   gboolean recheck_on_timeout;
+  gboolean retransmit_on_timeout;
   struct _CandidateCheckPair *discovered_pair;
   struct _CandidateCheckPair *succeeded_pair;
   guint64 priority;