4 files changed, 92 insertions, 75 deletions

diff --git a/src/or/circuit.c b/src/or/circuit.c
index 7208489fa..f591c11a8 100644
--- a/src/or/circuit.c
+++ b/src/or/circuit.c
@@ -205,18 +205,45 @@ circuit_t *circuit_get_by_conn(connection_t *conn) {
   return NULL;
 }
 
-circuit_t *circuit_get_newest_open(void) {
-  circuit_t *circ, *bestcirc=NULL;
+/* Find the newest circ that conn can use, preferably one which is
+ * dirty and not too old.
+ * If !conn, return newest open.
+ */
+circuit_t *circuit_get_newest_open(connection_t *conn) {
+  circuit_t *circ, *newest=NULL, *leastdirty=NULL;
 
   for(circ=global_circuitlist;circ;circ = circ->next) {
-    if(circ->cpath && circ->state == CIRCUIT_STATE_OPEN && circ->n_conn && (!bestcirc ||
-      bestcirc->timestamp_created < circ->timestamp_created)) {
-      log_fn(LOG_DEBUG,"Choosing circuit %s:%d:%d.", circ->n_conn->address, circ->n_port, circ->n_circ_id);
-      assert(circ->n_circ_id);
-      bestcirc = circ;
+    if(conn && connection_ap_can_use_exit(conn,
+       router_get_by_addr_port(circ->cpath->prev->addr, circ->cpath->prev->port)) < 0) {
+      log_fn(LOG_DEBUG,"Skipping %s:%d:%d because we couldn't exit there.",
+             circ->n_conn->address, circ->n_port, circ->n_circ_id);
+      continue;
     }
+    if(circ->cpath && circ->state == CIRCUIT_STATE_OPEN && circ->n_conn) {
+      if(!newest || newest->timestamp_created < circ->timestamp_created) {
+        assert(circ->n_circ_id);
+        newest = circ;
+      }
+      if(conn && circ->timestamp_dirty &&
+         (!leastdirty || leastdirty->timestamp_dirty < circ->timestamp_dirty)) {
+        assert(circ->n_circ_id);
+        leastdirty = circ;
+      }
+    }
+  }
+
+  if(leastdirty &&
+     leastdirty->timestamp_dirty+options.NewCircuitPeriod > time(NULL)) {
+    log_fn(LOG_DEBUG,"Choosing in-use circuit %s:%d:%d.",
+           leastdirty->n_conn->address, leastdirty->n_port, leastdirty->n_circ_id);
+    return leastdirty;
+  }
+  if(newest) {
+    log_fn(LOG_DEBUG,"Choosing circuit %s:%d:%d.",
+           newest->n_conn->address, newest->n_port, newest->n_circ_id);
+    return newest;
   }
-  return bestcirc;
+  return NULL;
 }
 
 int circuit_deliver_relay_cell(cell_t *cell, circuit_t *circ,
@@ -491,13 +518,8 @@ int circuit_consider_sending_sendme(circuit_t *circ, int edge_type, crypt_path_t
 
 void circuit_close(circuit_t *circ) {
   connection_t *conn;
-  circuit_t *youngest=NULL;
 
   assert(circ);
-  if(options.SocksPort) {
-    youngest = circuit_get_newest_open();
-    log_fn(LOG_DEBUG,"youngest %d, circ %d.",(int)youngest, (int)circ);
-  }
   circuit_remove(circ);
   if(circ->n_conn)
     connection_send_destroy(circ->n_circ_id, circ->n_conn);
@@ -509,11 +531,6 @@ void circuit_close(circuit_t *circ) {
   for(conn=circ->p_streams; conn; conn=conn->next_stream) {
     connection_send_destroy(circ->p_circ_id, conn); 
   }
-  if(options.SocksPort && youngest == circ) { /* check this after we've sent the destroys, to reduce races */
-    /* our current circuit just died. Launch another one pronto. */
-    log_fn(LOG_INFO,"Youngest circuit dying. Launching a replacement.");
-    circuit_launch_new(1);
-  }
   circuit_free(circ);
 }
 
@@ -606,15 +623,15 @@ void circuit_dump_by_conn(connection_t *conn, int severity) {
 
 void circuit_expire_unused_circuits(void) {
   circuit_t *circ, *tmpcirc;
-  circuit_t *youngest;
-
-  youngest = circuit_get_newest_open();
+  time_t now = time(NULL);
 
   circ = global_circuitlist;
   while(circ) {
     tmpcirc = circ;
     circ = circ->next;
-    if(tmpcirc != youngest && !tmpcirc->p_conn && !tmpcirc->p_streams) {
+    if(tmpcirc->timestamp_dirty &&
+       tmpcirc->timestamp_dirty + options.NewCircuitPeriod < now &&
+       !tmpcirc->p_conn && !tmpcirc->p_streams) {
       log_fn(LOG_DEBUG,"Closing n_circ_id %d",tmpcirc->n_circ_id);
       circuit_close(tmpcirc);
     }
@@ -624,34 +641,33 @@ void circuit_expire_unused_circuits(void) {
 /* failure_status code: negative means reset failures to 0. Other values mean
  * add that value to the current number of failures, then if we don't have too
  * many failures on record, try to make a new circuit.
+ *
+ * Return -1 if you aren't going to try to make a circuit, 0 if you did try.
  */
-void circuit_launch_new(int failure_status) {
+int circuit_launch_new(int failure_status) {
   static int failures=0;
 
   if(!options.SocksPort) /* we're not an application proxy. no need for circuits. */
-    return;
+    return -1;
 
   if(failure_status == -1) { /* I was called because a circuit succeeded */
     failures = 0;
-    return;
+    return -1;
   }
 
   failures += failure_status;
 
-retry_circuit:
-
   if(failures > 5) {
-    log_fn(LOG_INFO,"Giving up for now, %d failures.", failures);
-    return;
+    return -1;
   }
 
   if(circuit_establish_circuit() < 0) {
     failures++;
-    goto retry_circuit;
+    return 0;
   }
 
   failures = 0;
-  return;
+  return 0;
 }
 
 int circuit_establish_circuit(void) {
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 2274ca893..32fa77edd 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -18,7 +18,6 @@ static void connection_edge_consider_sending_sendme(connection_t *conn);
 
 static uint32_t client_dns_lookup_entry(const char *address);
 static void client_dns_set_entry(const char *address, uint32_t val);
-static void client_dns_clean(void);
 
 int connection_edge_process_inbuf(connection_t *conn) {
 
@@ -255,7 +254,8 @@ int connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, connection
         addr = ntohl(*cell->payload+RELAY_HEADER_SIZE+1);
         client_dns_set_entry(conn->socks_request->address, addr);
         conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
-        /* XXX Build another circuit as required */
+        if(connection_ap_handshake_attach_circuit(conn) < 0)
+          circuit_launch_new(1); /* Build another circuit to handle this stream */
         return 0;
       }
       log_fn(LOG_INFO,"end cell (%s) for stream %d. Removing stream.",
@@ -489,10 +489,12 @@ void connection_ap_attach_pending(void)
     if (carray[i]->type != CONN_TYPE_AP || 
         carray[i]->type != AP_CONN_STATE_CIRCUIT_WAIT)
       continue;
-    if (connection_ap_handshake_attach_circuit(carray[i])) {
-      need_new_circuit = 1; /* XXX act on this. */
+    if (connection_ap_handshake_attach_circuit(carray[i])<0) {
+      need_new_circuit = 1;
     }
   }
+  if(need_new_circuit)
+    circuit_launch_new(1);
 }
 
 static void connection_edge_consider_sending_sendme(connection_t *conn) {
@@ -547,51 +549,37 @@ static int connection_ap_handshake_process_socks(connection_t *conn) {
   } /* else socks handshake is done, continue processing */
 
   conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
-  if (connection_ap_handshake_attach_circuit(conn)) {
-    /* XXX we need a circuit */
-  }      
+  if (connection_ap_handshake_attach_circuit(conn)<0) {
+    circuit_launch_new(1);
+  }
   return 0;
 }
 
 /* Try to find a live circuit.  If we don't find one, tell 'conn' to
  * stop reading and return 0.  Otherwise, associate the CONN_TYPE_AP
- * connection 'conn' with the newest live circuit, start sending a
+ * connection 'conn' with a safe live circuit, start sending a
  * BEGIN cell down the circuit, and return 1.
  */
 static int connection_ap_handshake_attach_circuit(connection_t *conn) {
   circuit_t *circ;
-  
+
   assert(conn);
   assert(conn->type == CONN_TYPE_AP);
   assert(conn->state == AP_CONN_STATE_CIRCUIT_WAIT);
   assert(conn->socks_request);
-  
+
   /* find the circuit that we should use, if there is one. */
-  circ = circuit_get_newest_open();
+  circ = circuit_get_newest_open(conn);
 
   if(!circ) {
-    log_fn(LOG_INFO,"No circuit ready for edge connection; delaying.");
-    connection_stop_reading(conn);
-    /* XXX both this and the start_reading below can go away if we
-     *     remove our notion that we shouldn't read from a socks
-     *     client until we're connected. the socks spec promises that it
-     *     won't write. is that good enough?
-     */
-    return -1;
-  }
-  if (connection_ap_can_use_exit(conn,
-                  router_get_by_addr_port(circ->cpath->prev->addr,
-                                          circ->cpath->prev->port)) < 0) {
-    /* The exit on this circuit will _definitely_ reject this connection. */
-    log_fn(LOG_INFO,"Most recent circuit will reject AP connection; delaying.");
-    /* XXX Build another circuit. */
-    connection_stop_reading(conn);
+    log_fn(LOG_INFO,"No safe circuit ready for edge connection; delaying.");
+    connection_stop_reading(conn); /* don't read until the connected cell arrives */
     return -1;
   }
 
   connection_start_reading(conn);
 
-  circ->dirty = 1;
+  circ->timestamp_dirty = time(NULL);
 
   /* add it into the linked list of streams on this circuit */
   log_fn(LOG_DEBUG,"attaching new conn to circ. n_circ_id %d.", circ->n_circ_id);
@@ -893,7 +881,7 @@ static void client_dns_set_entry(const char *address, uint32_t val)
   }
 }
 
-static void client_dns_clean(void)
+void client_dns_clean(void)
 {
   struct client_dns_entry **expired_entries;
   int n_expired_entries = 0;
diff --git a/src/or/main.c b/src/or/main.c
index ce6d22b17..ad3c1304f 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -335,20 +335,31 @@ static void run_scheduled_events(time_t now) {
     time_to_fetch_directory = now + options.DirFetchPostPeriod;
   }
 
-  /* 2. Every NewCircuitPeriod seconds, we expire old circuits and make a 
-   *    new one as needed.
+  /* 2. Every second, we try a new circuit if there are no valid
+   *    circuits. Every NewCircuitPeriod seconds, we expire circuits
+   *    that became dirty more than NewCircuitPeriod seconds ago,
+   *    and we make a new circ if there are no clean circuits.
    */
-  if(options.SocksPort && time_to_new_circuit < now) {
-    circuit_expire_unused_circuits();
-    circuit_launch_new(-1); /* tell it to forget about previous failures */
-    circ = circuit_get_newest_open();
-    if(!circ || circ->dirty) {
-      log_fn(LOG_INFO,"Youngest circuit %s; launching replacement.", circ ? "dirty" : "missing");
-      circuit_launch_new(0); /* make an onion and lay the circuit */
+  if(options.SocksPort) {
+    circ = circuit_get_newest_open(NULL);
+    if(time_to_new_circuit < now) {
+      client_dns_clean();
+      circuit_expire_unused_circuits();
+      circuit_launch_new(-1); /* tell it to forget about previous failures */
+      if(circ && circ->timestamp_dirty) {
+        log_fn(LOG_INFO,"Youngest circuit dirty; launching replacement.");
+        circuit_launch_new(0); /* make a new circuit */
+      }
+      time_to_new_circuit = now + options.NewCircuitPeriod;
     }
-    time_to_new_circuit = now + options.NewCircuitPeriod;
+    if(!circ) {
+      circuit_launch_new(1);
+    }
+    /* XXX also check if we have any circuit_pending streams and we're not
+     * currently building a circuit for them.
+     */
   }
-  
+
   /* 3. Every second, we check how much bandwidth we've consumed and 
    *    increment global_read_bucket.
    */
@@ -358,7 +369,6 @@ static void run_scheduled_events(time_t now) {
     log_fn(LOG_DEBUG,"global_read_bucket now %d.", global_read_bucket);
   }
   stats_prev_global_read_bucket = global_read_bucket;
-  
 
   /* 4. We do houskeeping for each connection... */
   for(i=0;i<nfds;i++) {
@@ -368,6 +378,8 @@ static void run_scheduled_events(time_t now) {
   /* 5. and blow away any connections that need to die. can't do this later
    * because we might open up a circuit and not realize we're about to cull
    * the connection it's running over.
+   * XXX we can remove this step once we audit circuit-building to make sure
+   *     it doesn't pick a marked-for-close conn. -RD
    */
   for(i=0;i<nfds;i++)
     conn_close_if_marked(i);
diff --git a/src/or/or.h b/src/or/or.h
index e1da48093..44888f9fa 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -419,7 +419,7 @@ struct circuit_t {
 
   char onionskin[DH_ONIONSKIN_LEN]; /* for storage while onionskin pending */
   long timestamp_created;
-  uint8_t dirty; /* whether this circuit has been used yet */
+  long timestamp_dirty; /* when the circuit was first used, or 0 if clean */
 
   uint8_t state;
 
@@ -515,7 +515,7 @@ void circuit_free_cpath(crypt_path_t *cpath);
 circuit_t *circuit_enumerate_by_naddr_nport(circuit_t *start, uint32_t naddr, uint16_t nport);
 circuit_t *circuit_get_by_circ_id_conn(circ_id_t circ_id, connection_t *conn);
 circuit_t *circuit_get_by_conn(connection_t *conn);
-circuit_t *circuit_get_newest_open(void);
+circuit_t *circuit_get_newest_open(connection_t *conn);
 
 int circuit_deliver_relay_cell(cell_t *cell, circuit_t *circ,
                                int cell_direction, crypt_path_t *layer_hint);
@@ -533,7 +533,7 @@ void circuit_about_to_close_connection(connection_t *conn);
 void circuit_dump_by_conn(connection_t *conn, int severity);
 
 void circuit_expire_unused_circuits(void);
-void circuit_launch_new(int failure_status);
+int circuit_launch_new(int failure_status);
 int circuit_establish_circuit(void);
 void circuit_n_conn_open(connection_t *or_conn);
 int circuit_send_next_onion_skin(circuit_t *circ);
@@ -631,6 +631,7 @@ extern uint64_t stats_n_data_cells_received;
 extern uint64_t stats_n_data_bytes_received;
 
 void client_dns_init(void);
+void client_dns_clean(void);
 
 /********************************* connection_or.c ***************************/