commit e8b9815711e7cd1ef0b83153aefcc0d05e817f4e Author: Nick Mathewson nickm@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)