[tor-commits] [tor/master] Take a smarter approach to clearing isolation info

[nickm at torproject.org]

commit e8b9815711e7cd1ef0b83153aefcc0d05e817f4e
Author: Nick Mathewson <nickm at torproject.org>
Date:   Tue Jul 19 13:51:43 2011 -0400

    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.
---
 src/or/circuitlist.c     |   18 ++----------------
 src/or/circuituse.c      |   16 ++++++++++++++++
 src/or/connection_edge.c |    6 +++---
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index 28a7181..0fefe98 100644
--- 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 dcb6bfa..b486044 100644
--- 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 63779f2..4bbb080 100644
--- 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)