Commit e8b98157 authored by Nick Mathewson's avatar Nick Mathewson 🤹
Browse files

Take a smarter approach to clearing isolation info 

Back when I added this logic in 20c0581a, 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.
parent 12dfb4f5

src/or/circuitlist.c

+2 −16
Original line number Diff line number Diff line
@@ -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

src/or/circuituse.c

+16 −0
Original line number Diff line number Diff line
@@ -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.

src/or/connection_edge.c

+3 −3
Original line number Diff line number Diff line
@@ -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)