Take a smarter approach to clearing isolation info

Back when I added this logic in 20c0581a79, the rule was that whenever
a circuit finished building, we cleared its isolation info. I did that
so that we would still use the circuit even if all the streams that
had previously led us to tentatively set its isolation info had closed.

But there were problems with that approach: We could pretty easily get
into a case where S1 had led us to launch C1 and S2 had led us to
launch C2, but when C1 finished, we cleared its isolation and attached
S2 first.  Since C2 was still marked in a way that made S1
unattachable to it, we'd then launch another circuit needlessly.

So instead, we try the following approach now: when a circuit is done
building, we try to attach streams to it.  If it remains unused after
we try attaching streams, then we clear its isolation info, and try
again to attach streams.

Thanks to Sebastian for helping me figure this out.

diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index 28a7181f265c1bbe6a24edb75074f9f2371a30aa..0fefe9871e77b8c222d61ec8067f89a0b0e20043 100644 (file)
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -210,23 +210,9 @@ circuit_set_state(circuit_t *circ, uint8_t state)
     /* add to waiting-circuit list. */
     smartlist_add(circuits_pending_or_conns, circ);
   }
-
-  circ->state = state;
-
-  if (state == CIRCUIT_STATE_OPEN) {
+  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);
-      }
-    }
-  }
+  circ->state = state;
 }
 
 /** Add <b>circ</b> to the global list of circuits. This is called only from

diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index dcb6bfa50197a56f52037c19e0ebe3d996986ae9..b4860440cb0c0ff3e54e0db628b6634b5cf83243 100644 (file)
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -998,6 +998,7 @@ circuit_testing_failed(origin_circuit_t *circ, int at_last_hop)
 void
 circuit_has_opened(origin_circuit_t *circ)
 {
+  int can_try_clearing_isolation = 0, tried_clearing_isolation = 0;
   control_event_circuit_status(circ, CIRC_EVENT_BUILT, 0);
 
   /* Remember that this circuit has finished building. Now if we start
@@ -1005,9 +1006,12 @@ circuit_has_opened(origin_circuit_t *circ)
    * to consider its build time. */
   circ->has_opened = 1;
 
+ again:
+
   switch (TO_CIRCUIT(circ)->purpose) {
     case CIRCUIT_PURPOSE_C_ESTABLISH_REND:
       rend_client_rendcirc_has_opened(circ);
+      can_try_clearing_isolation = 1;
       connection_ap_attach_pending();
       break;
     case CIRCUIT_PURPOSE_C_INTRODUCING:
@@ -1016,6 +1020,7 @@ circuit_has_opened(origin_circuit_t *circ)
     case CIRCUIT_PURPOSE_C_GENERAL:
       /* Tell any AP connections that have been waiting for a new
        * circuit that one is ready. */
+      can_try_clearing_isolation = 1;
       connection_ap_attach_pending();
       break;
     case CIRCUIT_PURPOSE_S_ESTABLISH_INTRO:
@@ -1033,6 +1038,17 @@ circuit_has_opened(origin_circuit_t *circ)
      * This won't happen in normal operation, but might happen if the
      * controller did it. Just let it slide. */
   }
+
+  if (can_try_clearing_isolation && !tried_clearing_isolation &&
+      circ->isolation_values_set &&
+      !circ->isolation_any_streams_attached) {
+    /* If we have any isolation information set on this circuit, and
+     * we didn't manage to attach any streams to it, then we can
+     * and should clear it and try again. */
+    circuit_clear_isolation(circ);
+    tried_clearing_isolation = 1;
+    goto again;
+  }
 }
 
 /** Called whenever a circuit could not be successfully built.

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 63779f25cd85cc67c03b25e6b901e6f9a0c79781..4bbb08012476f801c4f287759db1e15b5b1d0de4 100644 (file)
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -3461,9 +3461,9 @@ connection_edge_update_circuit_isolation(const edge_connection_t *conn,
  * 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.)
+ * even more circuits.  Once the circuit is open, if it turns out that
+ * we no longer have any streams to attach to it, we clear the isolation flags
+ * and data so that other streams can have a chance.)
  */
 void
 circuit_clear_isolation(origin_circuit_t *circ)