[tor-commits] [tor/master] Clarify excess consensus connection cleanup by adding comments

[nickm at torproject.org]

commit 6057fb2f5bdda80b9056a750261f60ca2c960283
Author: teor (Tim Wilson-Brown) <teor2345 at gmail.com>
Date:   Sat Mar 26 11:08:39 2016 +1100

    Clarify excess consensus connection cleanup by adding comments
    
    Comment-only change
---
 src/or/directory.c | 44 +++++++++++++++++++++++++++++++++++---------
 src/or/main.c      | 10 ++++++++--
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/src/or/directory.c b/src/or/directory.c
index 99091b5..9139dbc 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -1161,6 +1161,8 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
         conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
         /* fall through */
       case 0:
+        /* Close this connection if there's another consensus connection
+         * downloading (during bootstrap), or connecting (after bootstrap). */
         if (connection_dir_close_consensus_conn_if_extra(conn)) {
           return;
         }
@@ -1211,8 +1213,8 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
       connection_mark_for_close(TO_CONN(conn));
       return;
     }
-    /* XXX the below line is suspicious and uncommented. does it close all
-     * consensus fetches if we've already bootstrapped? investigate. */
+    /* Close this connection if there's another consensus connection
+     * downloading (during bootstrap), or connecting (after bootstrap). */
     if (connection_dir_close_consensus_conn_if_extra(conn)) {
       return;
     }
@@ -3689,11 +3691,16 @@ connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose)
   return 0;
 }
 
-/* Check if we have excess consensus download connection attempts, and close
- * conn:
+/* Check if we have more than one consensus download connection attempt, and
+ * close conn:
  * - if we don't have a consensus, and we're downloading a consensus, and conn
- *   is not downloading a consensus yet, close it;
- * - if we do have a consensus, conn is excess, close it. */
+ *   is not downloading a consensus yet;
+ * - if we do have a consensus, and there's more than one consensus connection.
+ *
+ * Post-bootstrap consensus connection attempts are initiated one at a time.
+ * So this function won't close any consensus connection attempts that
+ * are initiated after bootstrap.
+ */
 int
 connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn)
 {
@@ -3710,6 +3717,10 @@ connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn)
     return 0;
   }
 
+  /* Only close this connection if there's another consensus connection
+   * downloading (during bootstrap), or connecting (after bootstrap).
+   * Post-bootstrap consensus connection attempts won't be closed, because
+   * they only occur one at a time. */
   if (!connection_dir_would_close_consensus_conn_helper()) {
     return 0;
   }
@@ -3733,15 +3744,28 @@ connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn)
   return 0;
 }
 
-/* Check if we have excess consensus download connection attempts, and close
- * them:
+/* Clean up excess consensus download connection attempts.
+ * During bootstrap, or when the bootstrap consensus has just been downloaded,
+ * if we have more than one active consensus connection:
  * - if we don't have a consensus, and we're downloading a consensus, keep an
  *   earlier connection, or a connection to a fallback directory, and close
  *   all other connections;
- * - if we do have a consensus, close all connections: they are all excess. */
+ * - if we have just downloaded the bootstrap consensus, and have other
+ *   consensus connections left over, close all of them.
+ *
+ * Post-bootstrap consensus connection attempts are initiated one at a time.
+ * So this function won't close any consensus connection attempts that
+ * are initiated after bootstrap.
+ */
 void
 connection_dir_close_extra_consensus_conns(void)
 {
+  /* Only cleanup connections if there is more than one consensus connection,
+   * and at least one of those connections is already downloading
+   * (during bootstrap), or connecting (just after the bootstrap consensus is
+   * downloaded).
+   * Post-bootstrap consensus connection attempts won't be cleaned up, because
+   * they only occur one at a time. */
   if (!connection_dir_would_close_consensus_conn_helper()) {
     return;
   }
@@ -3833,6 +3857,8 @@ connection_dir_finished_connecting(dir_connection_t *conn)
   log_debug(LD_HTTP,"Dir connection to router %s:%u established.",
             conn->base_.address,conn->base_.port);
 
+  /* Close this connection if there's another consensus connection
+   * downloading (during bootstrap), or connecting (after bootstrap). */
   if (connection_dir_close_consensus_conn_if_extra(conn)) {
     return -1;
   }
diff --git a/src/or/main.c b/src/or/main.c
index 00768ac..0bd3527 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -1485,8 +1485,14 @@ run_scheduled_events(time_t now)
   }
 
   /* 2d. Cleanup excess consensus bootstrap connections every second.
-   * connection_dir_close_consensus_conn_if_extra() will close connections
-   * that are clearly excess, but this check is more thorough. */
+   * connection_dir_close_consensus_conn_if_extra() closes some connections
+   * that are clearly excess, but this check is more thorough.
+   * This only closes connections if there is more than one consensus
+   * connection, and at least one of those connections is already downloading
+   * (during bootstrap), or connecting (just after the bootstrap consensus is
+   * downloaded).
+   * It won't close any consensus connections initiated after bootstrap,
+   * because those attempts are made one at a time. */
   connection_dir_close_extra_consensus_conns();
 
   /* 3a. Every second, we examine pending circuits and prune the