Launch sufficient circuits to satisfy pending isolated streams

Our old "do we need to launch a circuit for stream S" logic was,
more or less, that if we had a pending circuit that could handle S,
we didn't need to launch a new one.

But now that we have streams isolated from one another, we need
something stronger here: It's possible that some pending C can
handle either S1 or S2, but not both.

This patch reuses the existing isolation logic for a simple
solution: when we decide during circuit launching that some pending
C would satisfy stream S1, we "hypothetically" mark C as though S1
had been connected to it.  Now if S2 is incompatible with S1, it
won't be something that can attach to C, and so we'll launch a new
stream.

When the circuit becomes OPEN for the first time (with no streams
attached to it), we reset the circuit's isolation status.  I'm not
too sure about this part: I wanted some way to be sure that, if all
streams that would have used a circuit die before the circuit is
done, the circuit can still get used.  But I worry that this
approach could also lead to us launching too many circuits.  Careful
thought needed here.

src/or/circuitlist.c

@@ -210,9 +210,23 @@ circuit_set_state(circuit_t *circ, uint8_t state)
     /* add to waiting-circuit list. */
     smartlist_add(circuits_pending_or_conns, circ);
   }
-  if (state == CIRCUIT_STATE_OPEN)
-    tor_assert(!circ->n_conn_onionskin);
+
   circ->state = state;
+
+  if (state == CIRCUIT_STATE_OPEN) {
+    tor_assert(!circ->n_conn_onionskin);
+    if (CIRCUIT_IS_ORIGIN(circ)) {
+      origin_circuit_t *origin_circ = TO_ORIGIN_CIRCUIT(circ);
+      if (origin_circ->isolation_values_set &&
+          !origin_circ->isolation_any_streams_attached) {
+        /* If we have any isolation information set on this circuit,
+         * but we never attached any streams to it, then all of the
+         * isolation information was hypothetical.  Clear it.
+         */
+        circuit_clear_isolation(origin_circ);
+      }
+    }
+  }
 }
 
 /** Add <b>circ</b> to the global list of circuits. This is called only from

src/or/circuituse.c

@@ -1457,12 +1457,20 @@ circuit_get_open_circ_or_launch(edge_connection_t *conn,
           rend_client_rendcirc_has_opened(circ);
       }
     }
-  }
-  if (!circ)
+  } /* endif (!circ) */
+  if (circ) {
+    /* Mark the circuit with the isolation fields for this connection.
+     * When the circuit arrives, we'll clear these flags: this is
+     * just some internal bookkeeping to make sure that we have
+     * launched enough circuits.
+     */
+    connection_edge_update_circuit_isolation(conn, circ, 0);
+  } else {
     log_info(LD_APP,
              "No safe circuit (purpose %d) ready for edge "
              "connection; delaying.",
              desired_circuit_purpose);
+  }
   *circp = circ;
   return 0;
 }
@@ -1509,6 +1517,7 @@ link_apconn_to_circ(edge_connection_t *apconn, origin_circuit_t *circ,
     apconn->cpath_layer = circ->cpath->prev;
   }
 
+  circ->isolation_any_streams_attached = 1;
   connection_edge_update_circuit_isolation(apconn, circ, 0);
 }
 

src/or/connection_edge.c

@@ -3414,3 +3414,38 @@ connection_edge_update_circuit_isolation(const edge_connection_t *conn,
   }
 }
 
+/**
+ * Clear the isolation settings on <b>circ</b>.
+ *
+ * This only works on an open circuit that has never had a stream attached to
+ * it, and whose isolation settings are hypothetical.  (We set hypothetical
+ * isolation settings on circuits as we're launching them, so that we
+ * know whether they can handle more streams or whether we need to launch
+ * even more circuits.  We clear the flags once the circuits are open,
+ * in case the streams that made us launch the circuits have closed
+ * since we began launching the circuits.)
+ */
+void
+circuit_clear_isolation(origin_circuit_t *circ)
+{
+  if (circ->isolation_any_streams_attached) {
+    log_warn(LD_BUG, "Tried to clear the isolation status of a dirty circuit");
+    return;
+  }
+  if (TO_CIRCUIT(circ)->state != CIRCUIT_STATE_OPEN) {
+    log_warn(LD_BUG, "Tried to clear the isolation status of a non-open "
+             "circuit");
+    return;
+  }
+
+  circ->isolation_values_set = 0;
+  circ->isolation_flags_mixed = 0;
+  circ->client_proto_type = 0;
+  circ->client_proto_socksver = 0;
+  circ->dest_port = 0;
+  tor_addr_make_unspec(&circ->client_addr);
+  tor_free(circ->dest_address);
+  circ->session_group = -1;
+  circ->nym_epoch = 0;
+}
+

src/or/connection_edge.h

@@ -110,6 +110,7 @@ int connection_edge_compatible_with_circuit(const edge_connection_t *conn,
 int connection_edge_update_circuit_isolation(const edge_connection_t *conn,
                                              origin_circuit_t *circ,
                                              int dry_run);
+void circuit_clear_isolation(origin_circuit_t *circ);
 
 #endif
 

src/or/or.h

@@ -2461,9 +2461,19 @@ typedef struct origin_circuit_t {
   /* XXXX NM This can get re-used after 2**32 circuits. */
   uint32_t global_identifier;
 
-  /** True if we have attached at least one stream to this circuit, thereby
-   * setting the isolation paramaters for this circuit. */
+  /** True if we have associated one stream to this circuit, thereby setting
+   * the isolation paramaters for this circuit.  Note that this doesn't
+   * necessarily mean that we've <em>attached</em> any streams to the circuit:
+   * we may only have marked up this circuit during the launch process.
+   */
   unsigned int isolation_values_set : 1;
+  /** True iff any stream has <em>ever</em> been attached to this circuit.
+   *
+   * In a better world we could use timestamp_dirty for this, but
+   * timestamp_dirty is far too overloaded at the moment.
+   */
+  unsigned int isolation_any_streams_attached : 1;
+
   /** A bitfield of ISO_* flags for every isolation field such that this
    * circuit has had streams with more than one value for that field
    * attached to it. */
@@ -2471,11 +2481,15 @@ typedef struct origin_circuit_t {
 
   /** @name Isolation parameters
    *
-   * If any streams have been attached to this circuit (isolation_values_set
-   * == 1), and all streams attached to the circuit have had the same value
-   * for some field ((isolation_flags_mixed & ISO_FOO) == 0), then these
+   * If any streams have been associated with this circ (isolation_values_set
+   * == 1), and all streams associated with the circuit have had the same
+   * value for some field ((isolation_flags_mixed & ISO_FOO) == 0), then these
    * elements hold the value for that field.
    *
+   * Note again that "associated" is not the same as "attached": we
+   * preliminarily associate streams with a circuit while the circuit is being
+   * launched, so that we can tell whether we need to launch more circuits.
+   *
    * @{
    */
   uint8_t client_proto_type;